Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple SERVER Releases #1141

Merged
merged 4 commits into from
Mar 11, 2024
Merged

Multiple SERVER Releases #1141

merged 4 commits into from
Mar 11, 2024

Conversation

ravindk89
Copy link
Collaborator

@ravindk89 ravindk89 commented Feb 29, 2024

MinIO SERVER RELEASE.2024-01-05T22-17-24Z - added new metrics to github.com/minio/minio for later sync

MinIO SERVER RELEASE.2024-01-28T22-35-53Z - MinIO preallocates memory, mc update compresses binary in transit MinIO SERVER RELEASE.2024-02-06T21-36-22Z -

MinIO adds condition key for restricting STS AssumeRoleWithWebIdentity duration at policy level

Closes #1124 ,

Partially addresses #1116
Partially Addresses #1105

Staged:

I think that's everything.

The deploy tutorials could be more refined but I'd do a complete overhaul vs just patching something in for the quick term.

For K8s + the memory alloc, I thought about it but AFAIK we restrict to 4GiB per node of memory anyways, so this limit should not come up either way.

MinIO SERVER RELEASE.2024-01-05T22-17-24Z - added new metrics to github.com/minio/minio for later sync
MinIO SERVER RELEASE.2024-01-28T22-35-53Z - MinIO preallocates memory, mc update compresses binary in transit
MinIO SERVER RELEASE.2024-02-06T21-36-22Z - MinIO adds condition key for restricting STS AssumeRoleWithWebIdentity duration at policy level
@ravindk89 ravindk89 added the Review Assigned Reviewers Must LGTM To Close label Feb 29, 2024
@ravindk89 ravindk89 requested review from feorlen and djwfyi February 29, 2024 20:19
@ravindk89 ravindk89 self-assigned this Feb 29, 2024
Copy link
Collaborator

@djwfyi djwfyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.
A few suggestions to consider. And the metrics page has some unreadable tables.

I'll take another look after the next pass just to verify readability.


Specify a numeric value to limit the duration of *all* Security Token Service credentials generated by :ref:`minio-sts-assumerolewithwebidentity`.

This value overrides the ``DurationSeconds`` field specified to the client.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point of clarity. If the client specifies a shorter duration, which duration wins? This one? Or the client one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question - @vadmeste , I'm assuming the shorter value always wins regardless of where it's set.

That is, if I set the duration field here to be higher, then setting Duration to be lower in the STS API endpoint should work.

source/operations/checklists/hardware.rst Outdated Show resolved Hide resolved
@@ -17,6 +17,9 @@ Starting with :minio-release:`RELEASE.2022-06-02T02-11-04Z`, MinIO implements a
This feature allows access to :ref:`erasure coding dependent features <minio-erasure-coding>` without the requirement of multiple drives.
This mode **requires** accessing stored objects through the S3 API, and does **not** support direct access to objects through the filesystem/POSIX interface.

Starting with :minio-release:`RELEASE.2024-01-28T22-35-53Z`, MinIO pre-allocates 1GiB of system memory at startup.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is two paragraphs in a row that start this way.
I suggest moving this down into its own "Memory Requirements" subheading to match with the other pages, like what you did on the SNMD page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I did this mostly because the SNSD page doesn't otherwise have a pre-requisites section.

I guess I could add it in 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also ILM Metrics and Replication Metrics tables.
The metric names are too wide and the column with the description is squished to make the text really difficult or impossible to read.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use definition lists for metrics instead of tables? Metrics with hideously long names aren't going away. 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're including this from list.md, which is a markdown table.

I'll try to play with the SCSS and see if I can force something here.

source/reference/minio-mc-admin/mc-admin-update.rst Outdated Show resolved Hide resolved
@ravindk89 ravindk89 requested a review from djwfyi March 8, 2024 21:30
Copy link
Collaborator

@djwfyi djwfyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics are still a mess, but that's not really in the scope of this PR.
Everything else is good from what I see.

Merge it!

Copy link
Collaborator

@feorlen feorlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

(Markdown table sadness is a project for another day.)

@ravindk89 ravindk89 merged commit 67e81ce into main Mar 11, 2024
@ravindk89 ravindk89 deleted the RELEASE-WORK branch March 11, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review Assigned Reviewers Must LGTM To Close
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RELEASE] Server RELEASE.2024-02-06T21-36-22Z
3 participants