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

Show node's role in 'skuba cluster status' #928

Merged
merged 1 commit into from
Jan 27, 2020
Merged

Conversation

suseclee
Copy link
Contributor

Why is this PR needed?

Does it fix an issue? addresses a business case?skuba cluster status should show if a node is a master or worker (Daimler and T-Systems requested this).

What does this PR do?

Sample output:

 skuba cluster status
** This is an UNTAGGED version and NOT intended for production usage. **
NAME       ROLE     OS-IMAGE                              KERNEL-VERSION           KUBELET-VERSION   CONTAINER-RUNTIME   HAS-UPDATES   HAS-DISRUPTIVE-UPDATES   CAASP-RELEASE-VERSION
master-0   master   SUSE Linux Enterprise Server 15 SP1   4.12.14-197.29-default   v1.16.2           cri-o://1.16.1      yes           no                       <none>
master-1   master   SUSE Linux Enterprise Server 15 SP1   4.12.14-197.29-default   v1.16.2           cri-o://1.16.1      <none>        <none>                   <none>
worker-0   worker   SUSE Linux Enterprise Server 15 SP1   4.12.14-197.29-default   v1.16.2           cri-o://1.16.1      yes           no                       <none>
worker-1   worker   SUSE Linux Enterprise Server 15 SP1   4.12.14-197.29-default   v1.16.2           cri-o://1.16.1      yes           no                       <none>

Anything else a reviewer needs to know?

This is the indication of master node from kubectl get node -o json.
"labels": {
"beta.kubernetes.io/arch": "amd64",
"beta.kubernetes.io/os": "linux",
"kubernetes.io/arch": "amd64",
"kubernetes.io/hostname": "master-0",
"kubernetes.io/os": "linux",
"node-role.kubernetes.io/master": ""
},
In the case of worker nodes, there is no label.

Info for QA

This is info for QA so that they can validate this. This is mandatory if this PR fixes a bug.
If this is a new feature, a good description in "What does this PR do" may be enough.

Related info

Info that can be relevant for QA:

  • link to other PRs that should be merged together
  • link to packages that should be released together
  • upstream issues

Status BEFORE applying the patch

How can we reproduce the issue? How can we see this issue? Please provide the steps and the prove
this issue is not fixed.

Status AFTER applying the patch

How can we validate this issue is fixed? Please provide the steps and the prove this issue is fixed.

Docs

If docs need to be updated, please add a link to a PR to https://github.com/SUSE/doc-caasp.
At the time of creating the issue, this PR can be work in progress (set its title to [WIP]),
but the documentation needs to be finalized before the PR can be merged.

Merge restrictions

(Please do not edit this)

We are in v4-maintenance phase, so we will restrict what can be merged to prevent unexpected surprises:

What can be merged (merge criteria):
    2 approvals:
        1 developer: code is fine
        1 QA: QA is fine
    there is a PR for updating documentation (or a statement that this is not needed)

Klaven
Klaven previously approved these changes Jan 24, 2020
Copy link

@Klaven Klaven left a comment

Choose a reason for hiding this comment

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

I think this is fine.

Personally the entire community uses control-plane and not master anymore. I know the role stays that but we might want to think about changing the wording to be control-plane or likewise.

But other then that this loos good.

@dannysauer
Copy link
Contributor

Personally the entire community uses control-plane and not master anymore.

Without getting into a discussion of the merits of words, I agree that getting rid of slave but keeping master seems like a half measure in terms of making sure no one is offended.

However, considering how important consistency is, making this change here means we have a bunch of documentation that should be updated, and also means we should change the parameter passed to role (or at least accept both monikers). Are we ready to invest the resources to suppress the master term? I'm inclined to suggest doing that change to the whole codebase in one separate PR.

Copy link
Contributor

@dannysauer dannysauer left a comment

Choose a reason for hiding this comment

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

We're potentially suppressing information the customer assigned to nodes in this output. Requesting changes pending resolution of #928 (comment)

@Klaven
Copy link

Klaven commented Jan 27, 2020

Personally the entire community uses control-plane and not master anymore.

Without getting into a discussion of the merits of words, I agree that getting rid of slave but keeping master seems like a half measure in terms of making sure no one is offended.

However, considering how important consistency is, making this change here means we have a bunch of documentation that should be updated, and also means we should change the parameter passed to role (or at least accept both monikers). Are we ready to invest the resources to suppress the master term? I'm inclined to suggest doing that change to the whole codebase in one separate PR.

so the role that is assigned as a label will stay master there was a long talk on it. the only thing upstream changed was how they talked about it... aka, the control plane is a documentation/conversation. label == master. I am on board for leaving this master as it is consistent with upstream kubectl as well as what the label actually is. I do think our documentation should change to match upstream documentation.

so in the end. i'm fine with master and <none> just like kubectl

@suseclee
Copy link
Contributor Author

suseclee commented Jan 27, 2020

The output will be:

skuba cluster status
** This is an UNTAGGED version and NOT intended for production usage. **
NAME       ROLE     OS-IMAGE                              KERNEL-VERSION           KUBELET-VERSION   CONTAINER-RUNTIME   HAS-UPDATES   HAS-DISRUPTIVE-UPDATES   CAASP-RELEASE-VERSION
master-0   master   SUSE Linux Enterprise Server 15 SP1   4.12.14-197.29-default   v1.16.2           cri-o://1.16.1      yes           no                       <none>
worker-0   <none>   SUSE Linux Enterprise Server 15 SP1   4.12.14-197.29-default   v1.16.2           cri-o://1.16.1      yes           no                       <none>
worker-1   <none>   SUSE Linux Enterprise Server 15 SP1   4.12.14-197.29-default   v1.16.2           cri-o://1.16.1      yes           no                       <none>
worker-2   <none>   SUSE Linux Enterprise Server 15 SP1   4.12.14-197.29-default   v1.16.2           cri-o://1.16.1      yes           no                       <none>

Copy link

@Klaven Klaven left a comment

Choose a reason for hiding this comment

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

looking good!

pkg/skuba/actions/cluster/status/status.go Outdated Show resolved Hide resolved
pkg/skuba/actions/cluster/status/status.go Show resolved Hide resolved
Copy link
Contributor

@cmurphy cmurphy left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@dannysauer dannysauer left a comment

Choose a reason for hiding this comment

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

I'm approving either way, but we really should avoid using "caasp" in things like the source code and examples. The public name is caas; caasp is supposed to be just internal (even though it's scattered all over the place). I would've preferred to see the fake label's name use skuba, but that's really just me being unnecessarily picky. So, approved.

@suseclee suseclee merged commit 6352f37 into SUSE:master Jan 27, 2020
@dannysauer dannysauer added 4.1.2 enhancement New feature or request labels Jan 30, 2020
@jordimassaguerpla
Copy link
Member

Do we have a bsc number for this?

@suseclee
Copy link
Contributor Author

Do we have a bsc number for this?

Sorry for being late. We had a holiday on Monday. The number is #1140530: https://bugzilla.suse.com/show_bug.cgi?id=1140530

@jordimassaguerpla
Copy link
Member

Thanks. I see this is refered in https://github.com/SUSE/avant-garde/issues/545 which is already closed and in staging. All matches now. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.1.2 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants