-
Notifications
You must be signed in to change notification settings - Fork 1.9k
updating feature gates docs #24132
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
updating feature gates docs #24132
Conversation
fb8a7fd to
f07a670
Compare
|
@sttts Here is a second attempt at reworking the feature gates docs. WDYT? |
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.
how do I know the set of feature gates I can set?
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.
there is no easier way than this to reach a config CR? @jhadvig can you confirm?
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.
same as above: this needs a link to all possible values.
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.
double "enables"
|
Nits and missing links. |
|
@xltian Can you please assign this issue to a QE resource for review? Thank you! |
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.
from https://github.com/openshift/api/blob/master/config/v1/types_feature.go, I saw "IPv6DualStackNoUpgrade enables dual-stack. Turning this feature set on IS NOT SUPPORTED". Since turn this feature set on is not supported, we should remove IPv6DualStackNoUpgrade feature set.
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.
@sttts ^^
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.
cc networking team @danwinship
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.
I'm not sure what the rules about documenting feature sets are. In this case, we don't want the featureset to be used by people who don't already know about it, so not documenting it seems like the right thing.
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.
from https://docs.openshift.com/container-platform/4.5/nodes/clusters/nodes-cluster-enabling-features.html#feature-gate-features_nodes-cluster-enabling ,do we need add Technology Preview features like MachineHealthCheck and LocalStorageCapacityIsolation?
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.
@lyman9966 @sttts The list came from here: https://github.com/openshift/api/blob/release-4.6/config/v1/types_feature.go#L29
Stefan, these are the 4 feature sets that the user can enable?
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.
@mburke5678 I think you misunderstood me. "https://docs.openshift.com/container-platform/4.5/nodes/clusters/nodes-cluster-enabling-features.html#feature-gate-features_nodes-cluster-enabling" show us there are 4 features affected by feature set "TechPreviewNoUpgrade", such as "RotateKubeletServerCertificate","SupportPodPidsLimit","MachineHealthCheck","LocalStorageCapacityIsolation". Yet your doc lack the latter two.
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.
@lyman9966 MachineHealthCheck went GA in 4.3: https://docs.openshift.com/container-platform/4.3/machine_management/deploying-machine-health-checks.html
It appears that LocalStorageCapacityIsolation was enabled at some point https://github.com/openshift/api/pull/462/files
I assume this means that the TechPreviewNoUpgrade feature gate would not enable these features.
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.
@mburke5678 , Okay, then "MachineHealthCheck" and "LocalStorageCapacityIsolation" doesn't belong to TechPreview feature, so they can be removed.
|
General question: why do we place feature gates under node configuration in the docs? We had a similar case with audit and moved it out from there. Both audit and features gates are cluster-wide settings, and have nothing to do with nodes in particular. |
@sttts I see the feature gates under Node->Working with clusters chapter, It seems in 4.x, we move cluster-wide settings to Node chapter. |
|
@sttts I would guess that Feature Gates was added to nodes (likely by me) because in 3.x the user could activate feature gates by node in addition to the entire cluster. |
|
@sferich888 There has been discussion here about whether we want to document Feature Gates that enable unsupported features in the end user docs. What do you think? |
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 following is commented-out. No need for review at this time
|
@sferich888 There has been discussion here about whether we want to document Feature Gates that enable unsupported features in the end user docs. What do you think? |
|
@sferich888 There has been discussion here about whether we want to document Feature Gates that enable unsupported features in the end user docs. What do you think? |
|
@reestr Can you weigh in here in Eric Rich's absence? Do we want to document Feature Gates that enable unsupported features in the end user docs? |
e1a745b to
43206be
Compare
|
@sferich888 Do we want to document Feature Gates that enable unsupported features in the end user docs? |IPv6DualStackNoUpgrade @lyman9966 said: Turning this feature set on IS NOT SUPPORTED". Since turn this feature set on is not supported, we should remove IPv6DualStackNoUpgrade feature set. |
|
@mburke5678 documenting the 'available' feature gates 'supported' or 'unsupported' is fine, so long as we 'clearly' (boldly) call out what 'features' are 'experimental' and or 'risky' for production usecases. The term 'unsupported' (or inversely - supported) is a loaded term, because it means many things to many different people (or classes of users), so I would 'try and avoid it'. however in your 'statement' what you say is fine as your clearly outlining that enabling this / blocks upgrades. |
|
@sferich888 @lyman9966 I can add the term @lyman9966 Can I ask you to please take another look at the udated warnings for |
|
Deploy preview for osdocs ready! Built with commit b0191852e5ecca65b02d8a87d421fb176b09534a |
18d9386 to
81adaa2
Compare
|
New changes are detected. LGTM label has been removed. |
81adaa2 to
98153c1
Compare
98153c1 to
3d7df4a
Compare
lbarbeevargas
left a comment
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.
Great job on this! Just a few comments per the OCP guidelines and some small wording suggestions. :)
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.
If you are specifying say that this is a custom resource, what do you think of:
s/the Feature Gate custom resource (CR)/the FeatureGate custom resource (CR)/.
It is also in backticks in modules/nodes-cluster-enabling-features-cli.adoc.
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.
Yeah, I caught that one, but missed the others. D'oh!
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.
Similar comment as for the module intro above:
s/Feature Gate CR/FeatureGate CR/
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.
I think we recently switched from bold to italics to emphasize text. s/*not supported*/_not supported_/
Per the doc guidelines:
Use of underscores for general emphasis is allowed but should only be used very sparingly.
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.
I am now watching the Doc Guidelines. Hope I get notified of such changes!
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.
Does the feature set need to be in backticks here? s/SupportPodPidsLimit/SupportPodPidsLimit/
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.
@lbarbeevargas That is the name of a feature in the feature set. I don't think we mark-up feature names(??). We don't in Tech Preview notes, apparently: https://github.com/openshift/openshift-docs/pull/13878/files#diff-99eeb6425c261c0f96653039df58187866a5429c67f34e40c6364f0bd3d5e6a0L10
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.
@mburke5678 The feature names caught my attention here because they are in PascalCase, whereas the feature name (:FeatureName: variable) example linked in the guidelines is spelled out.
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.
@lbarbeevargas The Pascal case spelling is the common usage on the internet. But, the Kubernetes docs use Pascal case in the feature gates docs and Pod PID limits in the release notes.
What if we go with both, too? Pod PID limits (SupportPodPidsLimit)
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.
I haven't come across this situation before, but LGTM if it makes sense to the other reviewers. 👍 If going with the dual format, the first bullet can be updated as well:
link:https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet-tls-bootstrapping/#certificate-rotation[Certificate rotation (RotateKubeletServerCertificate)]. Enables the rotation of the server TLS certificate on the kubelet.
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.
s/Feature Gate CR/FeatureGate CR/
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.
Same feedback as for the CLI module: s/enable feature sets for all the nodes/enable feature sets for all of the nodes/
or
/enable feature sets for all nodes/
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.
Small nit for these two bullets:
IPv6DualStackNoUpgradeenables the dual-stack networking mode.TechPreviewNoUpgradeenables specific Technology Preview features.
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.
Small nit for these two bullets:
IPv6DualStackNoUpgradeenables the dual-stack networking mode.TechPreviewNoUpgradeenables specific Technology Preview features.
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.
Should the full assembly path be here? nodes/clusters/nodes-cluster-enabling-features.adoc
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.
Good catch. I moved these files around a lot when converting from 3.11 to 4.x. I'm sure there are others just like this,
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.
Should the full assembly path be here? nodes/clusters/nodes-cluster-enabling-features.adoc
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.
s/FeatureGates/feature gates/
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.
Removed
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.
Small rewording suggestion:
You can enable Technology Preview features for all nodes in your cluster by editing the
FeatureGatecustom resource in theopenshift-configproject.
|
Separate PR required for 4.5 as |
d1d10f4 to
7ea7680
Compare
|
the Topololgy Manager part LGTM. I mean for bug https://bugzilla.redhat.com/show_bug.cgi?id=1921160 |
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.
I don't think we want to remove this.
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.
@sferich888 I renamed this module for consistency. This sentence is still in the docs: https://github.com/openshift/openshift-docs/pull/24132/files#diff-17f46d9305040639ceb3dfbc0f5bc64625da1d3b3174750c4b2f377abaf90bb5R10
|
New TP features for 4.8?? |
b019185 to
d146cbf
Compare
https://bugzilla.redhat.com/show_bug.cgi?id=1860289
Reorganized the information; updated feature set list; added console instructions; renamed files for consistency.
Preview: https://deploy-preview-24132--osdocs.netlify.app/openshift-enterprise/latest/nodes/clusters/nodes-cluster-enabling-features.html
Wording for unsupported feature sets from: https://github.com/openshift/machine-config-operator/blob/master/vendor/github.com/openshift/api/config/v1/types_feature.go#L41
LatencySentitive feature set no longer required for Topology Manager per https://access.redhat.com/solutions/5807641 and https://bugzilla.redhat.com/show_bug.cgi?id=1921160
LatencySensitive feature set does not affect any other feature and can be removed from docs: https://coreos.slack.com/archives/CQNBUEVM2/p1618238903115900?thread_ts=1616327266.083200&cid=CQNBUEVM2