-
Notifications
You must be signed in to change notification settings - Fork 301
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
Multiple SERVER Releases #1141
Conversation
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
There was a problem hiding this 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/administration/identity-access-management/policy-based-access-control.rst
Outdated
Show resolved
Hide resolved
source/administration/identity-access-management/policy-based-access-control.rst
Outdated
Show resolved
Hide resolved
source/operations/install-deploy-manage/deploy-minio-multi-node-multi-drive.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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cluster replication metrics table is unreadable.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
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.
Co-authored-by: Daryl White <[email protected]>
There was a problem hiding this 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!
There was a problem hiding this 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.)
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.