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
test/e2e/framework/:use the term 'Control Plane' in comment #95053
Conversation
/release-note-none |
/assign @oomichi |
/wg naming |
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
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.
/lgtm
/hold |
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.
IMO a blind s/Master/Control Plane/ is not appropriate here
/cc @neolit123 @BenTheElder
for input on whether "control plane node" as a term makes sense
// This list of nodes must not include the Control Plane, which is marked | ||
// unschedulable, since the Control Plane doesn't run kube-proxy. Without |
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 list of nodes must not include the Control Plane, which is marked | |
// unschedulable, since the Control Plane doesn't run kube-proxy. Without | |
// This list of nodes must not include any control plane nodes, which are marked | |
// unschedulable, since control plane nodes don't run kube-proxy. Without |
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.
done
test/e2e/framework/node/resource.go
Outdated
@@ -191,7 +191,7 @@ func Filter(nodeList *v1.NodeList, fn func(node v1.Node) bool) { | |||
nodeList.Items = l | |||
} | |||
|
|||
// TotalRegistered returns number of registered Nodes excluding Master Node. | |||
// TotalRegistered returns number of registered Nodes excluding Control Plane Node. |
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.
// TotalRegistered returns number of registered Nodes excluding Control Plane Node. | |
// TotalRegistered returns number of schedulable Nodes |
test/e2e/framework/node/resource.go
Outdated
@@ -201,7 +201,7 @@ func TotalRegistered(c clientset.Interface) (int, error) { | |||
return len(nodes.Items), nil | |||
} | |||
|
|||
// TotalReady returns number of ready Nodes excluding Master Node. | |||
// TotalReady returns number of ready Nodes excluding Control Plane Node. |
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.
// TotalReady returns number of ready Nodes excluding Control Plane Node. | |
// TotalReady returns number of ready schedulable Nodes |
test/e2e/framework/node/wait.go
Outdated
@@ -41,7 +41,7 @@ var requiredPerNodePods = []*regexp.Regexp{ | |||
|
|||
// WaitForReadyNodes waits up to timeout for cluster to has desired size and | |||
// there is no not-ready nodes in it. By cluster size we mean number of Nodes | |||
// excluding Master Node. | |||
// excluding Control Plane Node. |
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 it's more appropriate to drop reference to master/control-plane entirely, e.g.
By cluster size we mean number of schedulable 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.
sure
test/e2e/framework/node/wait.go
Outdated
@@ -150,7 +150,7 @@ func WaitForNodeToBeReady(c clientset.Interface, name string, timeout time.Durat | |||
|
|||
// CheckReady waits up to timeout for cluster to has desired size and | |||
// there is no not-ready nodes in it. By cluster size we mean number of Nodes | |||
// excluding Master Node. | |||
// excluding Control Plane Node. |
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 it's more appropriate to drop reference to master/control-plane entirely, e.g.
By cluster size we mean number of schedulable Nodes
test/e2e/framework/ssh/ssh.go
Outdated
// NodeSSHHosts returns SSH-able host names for all schedulable nodes - this | ||
// excludes master node. If it can't find any external IPs, it falls back to | ||
// excludes Control Plane node. If it can't find any external IPs, it falls back to |
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 it's more appropriate to drop references to the master/control-plane entirely, e.g.
NodeSSHHosts returns SSH-able host names for all schedulable nodes. If it can't find any ....
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.
done. thanks for your comments.
/cc @spiffxp
@spiffxp your comments above are valid.
yes, it refers to a "node that hosts control-plane components". |
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.
/approve
/lgtm
/hold cancel
thanks for making the changes
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spiffxp, tanjunchen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
ref #94901
Special notes for your reviewer:
/cc @spiffxp
/cc @oomichi
test/e2e/framework/ingress/ingress_utils.go
test/e2e/framework/node/resource.go
test/e2e/framework/node/wait.go
test/e2e/framework/ssh/ssh.go
Currently these values only involve comments, so it is easy to replace.
Separate submission of these changes are conducive to review.
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: