CONSOLE-3243: Rename "master" to "control plane node" in node pages #11927

tvu20 · 2022-08-03T13:57:10Z

Addresses https://issues.redhat.com/browse/CONSOLE-3243

Changes all string occurences of "master" in the console to "control plane node".

tvu20 · 2022-08-04T14:50:53Z

/retest

jcaianirh · 2022-08-04T20:04:29Z

QE Approver:
/assign @yapei

Docs Approver:
/assign @opayne1

PX Approver:
/assign @RickJWagner

Console Approver:
/assign @rhamilto

RickJWagner

8 files reviewed, good.

RickJWagner · 2022-08-04T20:27:33Z

/label px-approved

openshift-ci · 2022-08-04T20:27:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RickJWagner, tvu20
Once this PR has been reviewed and has the lgtm label, please ask for approval from rhamilto by writing /assign @rhamilto in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adellape · 2022-08-04T20:54:01Z

/label docs-approved

rhamilto · 2022-08-05T14:26:09Z

Nice work, @tvu20! Also need to include updates to Cluster Settings:

console/frontend/public/module/k8s/cluster-settings.ts

Line 309 in 48991b5

Master = 'Master',

becomes Master = 'Control plane',

console/frontend/public/components/cluster-settings/cluster-settings.tsx

Lines 619 to 624 in 48991b5

    
           const machineConfigPoolIsEditable = useAccessReview({ 
        
             group: MachineConfigPoolModel.apiGroup, 
        
             resource: MachineConfigPoolModel.plural, 
        
             verb: 'patch', 
        
             name, 
        
           });

becomes

  const mcpName = machineConfigPool?.metadata?.name; 
  const machineConfigPoolIsEditable = useAccessReview({
    group: MachineConfigPoolModel.apiGroup,
    resource: MachineConfigPoolModel.plural,
    verb: 'patch',
    name: mcpName,
  });

console/frontend/public/components/cluster-settings/cluster-settings.tsx

Line 648 in 48991b5

to={`/k8s/cluster/nodes?rowFilter-node-role=${machineConfigPool?.metadata?.name}`}

becomes to={/k8s/cluster/nodes?rowFilter-node-role=${mcpName}}

It strikes me as a little odd that we're changing "master" to "control plane", but the underlying resource labels aren't changing. So we still have "master" in the UI. For example,

Screen.Recording.2022-08-05.at.10.17.35.AM.mov

Do we need to engage UXD here so as to avoid creating confusion for users? cc: @megan-hall

rhamilto · 2022-08-05T14:26:56Z

frontend/packages/console-app/src/components/nodes/NodesPage.tsx

@@ -512,7 +512,7 @@ const NodesPage = connect<{}, MapDispatchToProps>(
        items: [
          {
            id: 'master',
-            title: t('console-app~Master'),
+            title: t('console-app~Control plane node'),


Suggested change

title: t('console-app~Control plane node'),

title: t('console-app~Control plane'),

rhamilto · 2022-08-05T17:59:03Z

@tvu20, I think this probably merits a wider discussion. Unless the resource names (e.g., /k8s/cluster/machineconfiguration.openshift.iov1MachineConfigPool/master) and associated labels are changing, there is a limit to how far we can take these changes, and I fear it may create confusion on the part of end users. We probably need to revisit the intent of the story with @jhadvig and the stakeholder.

rhamilto · 2022-08-05T18:00:13Z

frontend/public/module/k8s/cluster-settings.ts

@@ -297,7 +297,7 @@ export const getNotUpgradeableResources = (resources) =>
  resources.filter((resource) => getConditionUpgradeableFalse(resource));

 export enum NodeTypes {
-  master = 'master',


This is problematic for

console/frontend/public/module/k8s/cluster-settings.ts

Line 313 in f4247fb

export const isMCPMaster = (mcp: MachineConfigPoolKind) => mcp.metadata.name === NodeTypes.master;

yapei · 2022-08-08T06:23:46Z

@XiyunZhao is testing on this PR

tvu20 · 2022-08-08T13:32:29Z

/retest

tvu20 · 2022-08-08T16:51:19Z

/retest

XiyunZhao · 2022-08-09T04:53:57Z

Hi @tvu20, based on current changes, I opened two bugs for this new feature, could you help to identify whether they need to update or not?

The name of "master" on "Filter by node type" dropdown list on cluster utilization on the overview page is not changed
https://bugzilla.redhat.com/show_bug.cgi?id=2116608
After the changes, duplicate role name will be listed on "Nodes" page which is incorrect
https://bugzilla.redhat.com/show_bug.cgi?id=2116625

tvu20 · 2022-08-09T18:23:17Z

Hi @XiyunZhao, I have just pushed a new commit that addresses bug 1. I am not able to replicate the second bug on my end.

tvu20 · 2022-08-11T13:27:48Z

/retest

tvu20 · 2022-08-11T14:01:12Z

/retest

Addresses https://issues.redhat.com/browse/CONSOLE-3243 Changes all string occurences of "master" in the console to "control plane node".

tvu20 · 2022-08-11T20:37:27Z

/retest

tvu20 · 2022-08-12T00:51:49Z

/retest

openshift-ci · 2022-08-12T02:29:31Z

@tvu20: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/frontend	`bf85b78`	link	true	`/test frontend`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

XiyunZhao · 2022-08-15T11:09:05Z

@tvu20 sorry for bug 2, after checking, it is not a duplicate value.
I would like to ask what's the difference between "control plane" with "control-plane"? Will it mislead customers?

jhadvig · 2022-08-15T14:47:21Z

@opayne1 could you please elaborate on the previous question? Thanks !

opayne1 · 2022-08-15T15:04:22Z

@jhadvig @XiyunZhao In our documentation, we use "control plane" with no hyphen. See the Control plane architecture docs for an example. We also have it as "control planes" no hyphen in our documentation contributor guidelines. Adding the docs-approved label as this PR looks good to me.

opayne1 · 2022-08-15T15:19:07Z

/label docs-approved

XiyunZhao · 2022-08-16T01:44:31Z

@tvu20, I think this probably merits a wider discussion. Unless the resource names (e.g., /k8s/cluster/machineconfiguration.openshift.iov1MachineConfigPool/master) and associated labels are changing, there is a limit to how far we can take these changes, and I fear it may create confusion on the part of end users. We probably need to revisit the intent of the story with @jhadvig and the stakeholder.

@tvu20 Is there any conclusion for this comment? Could you help to add some details to features description?

jhadvig · 2022-08-16T09:56:33Z

@tvu20, I think this probably merits a wider discussion. Unless the resource names (e.g., /k8s/cluster/machineconfiguration.openshift.iov1MachineConfigPool/master) and associated labels are changing, there is a limit to how far we can take these changes, and I fear it may create confusion on the part of end users. We probably need to revisit the intent of the story with @jhadvig and the stakeholder.

I agree with @rhamilto. We should revisit the story since it can lead to confusion between Nodes, MachineConfig and MachineConfigPool resources, which are still using the legacy master naming. Until that is solved, this change would only lead to user confusion. We should be consistent with the naming.

/hold

spadgett · 2022-08-16T14:19:02Z

I'm in favor of doing what we can to get away from this terminology, but I agree this will create confusion since the actual names and labels of the resources are still master. You'll need to know the real value when using the CLI, writing label selectors, or editing the resource YAML. Even in the UI, we would show "control plane" in some places and "master" in others as @rhamilto points out.

Have we considered fixing this at the platform level and changing the actual resource names? (I'm sure this is easier said than done since names are immutable, this is arguably API breaking, and we probably don't want to drift from upstream...)

@deads2k @sdodson Has this come up before?

sdodson · 2022-08-18T13:37:50Z

@deads2k @sdodson Has this come up before?

Poking around to see what I can find. This seems to have happened upstream via kubeadm(kubernetes/kubeadm#2200) but we won't pick those changes up in OCP as we don't leverage kubeadm.

spadgett · 2022-08-18T14:14:35Z

@sdodson has pointed out that we should at least be able to use a new node role in 4.12: https://bugzilla.redhat.com/show_bug.cgi?id=1995858 (thanks!)

spadgett · 2022-08-18T14:25:34Z

OK, here is the upstream guidance:

https://github.com/kubernetes/community/blob/master/sig-architecture/naming/recommendations/001-master-control-plane.md

Based on this, I'd suggest

We use "control plane" or "control plane nodes" in text descriptions, help, etc. rather than "master"
We use the node-role.kubernetes.io/control-plane role instead of node-role.kubernetes.io/master
We only use master when displaying a name of a resource

jcaianirh · 2022-10-18T23:39:44Z

@spadgett @rhamilto @jhadvig So based on Sams comment above, is this PR / story still needed or is this change a docs only or help only change? If this is a code change, can we push this to 4.13?

rhamilto · 2022-10-19T12:14:33Z

https://github.com/kubernetes/community/blob/master/sig-architecture/naming/recommendations/001-master-control-plane.md

Yes, we'll still need console code changes, most of which are in this PR, but we need a few tweaks based on the guidance.

rhamilto · 2022-10-25T20:00:06Z

Replaced by #12209

openshift-ci bot requested review from cyril-ui-developer and rawagner August 3, 2022 13:57

openshift-ci bot added component/core Related to console core functionality component/dashboard Related to dashboard component/pipelines Related to pipelines-plugin kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Aug 3, 2022

openshift-ci bot assigned opayne1, rhamilto, RickJWagner and yapei Aug 4, 2022

RickJWagner approved these changes Aug 4, 2022

View reviewed changes

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Aug 4, 2022

RickJWagner removed their assignment Aug 4, 2022

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Aug 4, 2022

rhamilto reviewed Aug 5, 2022

View reviewed changes

tvu20 force-pushed the CONSOLE-3243 branch from 26ec00b to f4247fb Compare August 5, 2022 15:14

openshift-ci bot added component/lso Related to local-storage-operator-plugin component/shared Related to console-shared labels Aug 5, 2022

rhamilto reviewed Aug 5, 2022

View reviewed changes

tvu20 force-pushed the CONSOLE-3243 branch from e5d4b4f to 6ed1e29 Compare August 5, 2022 18:35

tvu20 force-pushed the CONSOLE-3243 branch from 0f2b380 to d12ab74 Compare August 9, 2022 18:22

CONSOLE-3243: Rename "master" to "control plane node" in node pages

bf85b78

Addresses https://issues.redhat.com/browse/CONSOLE-3243 Changes all string occurences of "master" in the console to "control plane node".

tvu20 force-pushed the CONSOLE-3243 branch from d12ab74 to bf85b78 Compare August 11, 2022 16:14

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2022

rhamilto mentioned this pull request Oct 25, 2022

CONSOLE-3243: Rename "master" to "control plane node" in node pages #12209

Merged

rhamilto closed this Oct 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CONSOLE-3243: Rename "master" to "control plane node" in node pages #11927

CONSOLE-3243: Rename "master" to "control plane node" in node pages #11927

tvu20 commented Aug 3, 2022

tvu20 commented Aug 4, 2022

jcaianirh commented Aug 4, 2022

RickJWagner left a comment

RickJWagner commented Aug 4, 2022

openshift-ci bot commented Aug 4, 2022

adellape commented Aug 4, 2022

rhamilto commented Aug 5, 2022

rhamilto Aug 5, 2022

rhamilto commented Aug 5, 2022

rhamilto Aug 5, 2022

yapei commented Aug 8, 2022 •

edited

Loading

tvu20 commented Aug 8, 2022

tvu20 commented Aug 8, 2022

XiyunZhao commented Aug 9, 2022

tvu20 commented Aug 9, 2022

tvu20 commented Aug 11, 2022

tvu20 commented Aug 11, 2022

tvu20 commented Aug 11, 2022

tvu20 commented Aug 12, 2022

openshift-ci bot commented Aug 12, 2022

XiyunZhao commented Aug 15, 2022

jhadvig commented Aug 15, 2022

opayne1 commented Aug 15, 2022

opayne1 commented Aug 15, 2022

XiyunZhao commented Aug 16, 2022

jhadvig commented Aug 16, 2022

spadgett commented Aug 16, 2022

sdodson commented Aug 18, 2022

spadgett commented Aug 18, 2022

spadgett commented Aug 18, 2022

jcaianirh commented Oct 18, 2022 •

edited

Loading

rhamilto commented Oct 19, 2022

rhamilto commented Oct 25, 2022

	title: t('console-app~Control plane node'),
	title: t('console-app~Control plane'),

CONSOLE-3243: Rename "master" to "control plane node" in node pages #11927

CONSOLE-3243: Rename "master" to "control plane node" in node pages #11927

Conversation

tvu20 commented Aug 3, 2022

tvu20 commented Aug 4, 2022

jcaianirh commented Aug 4, 2022

RickJWagner left a comment

Choose a reason for hiding this comment

RickJWagner commented Aug 4, 2022

openshift-ci bot commented Aug 4, 2022

adellape commented Aug 4, 2022

rhamilto commented Aug 5, 2022

rhamilto Aug 5, 2022

Choose a reason for hiding this comment

rhamilto commented Aug 5, 2022

rhamilto Aug 5, 2022

Choose a reason for hiding this comment

yapei commented Aug 8, 2022 • edited Loading

tvu20 commented Aug 8, 2022

tvu20 commented Aug 8, 2022

XiyunZhao commented Aug 9, 2022

tvu20 commented Aug 9, 2022

tvu20 commented Aug 11, 2022

tvu20 commented Aug 11, 2022

tvu20 commented Aug 11, 2022

tvu20 commented Aug 12, 2022

openshift-ci bot commented Aug 12, 2022

XiyunZhao commented Aug 15, 2022

jhadvig commented Aug 15, 2022

opayne1 commented Aug 15, 2022

opayne1 commented Aug 15, 2022

XiyunZhao commented Aug 16, 2022

jhadvig commented Aug 16, 2022

spadgett commented Aug 16, 2022

sdodson commented Aug 18, 2022

spadgett commented Aug 18, 2022

spadgett commented Aug 18, 2022

jcaianirh commented Oct 18, 2022 • edited Loading

rhamilto commented Oct 19, 2022

rhamilto commented Oct 25, 2022

yapei commented Aug 8, 2022 •

edited

Loading

jcaianirh commented Oct 18, 2022 •

edited

Loading