staticpod: improve operator messages #913

mfojtik · 2020-10-07T08:25:20Z

Do proper pluralization in messages
Show total node count (so we can determine if all expected control plane nodes are there)
Unify on terminology ("achieve" vs. "is on")
Add messages to unit test to make sure we have them correct

openshift-ci-robot · 2020-10-07T08:25:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mfojtik
To complete the pull request process, please assign smarterclayton after the PR has been reviewed.
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

sttts · 2020-10-07T08:52:25Z

pkg/operator/staticpod/controller/installer/installer_controller.go

-		count := counts[currentRevision]
-		revisionStrings = append(revisionStrings, fmt.Sprintf("%d nodes are at revision %d", count, currentRevision))
+		if count := counts[currentRevision]; count == 1 {
+			revisionStrings = append(revisionStrings, fmt.Sprintf("1 node achieved revision %d", currentRevision))


is "achieved" the right word?

we should pick one and be consistent "are at" vs. "achieved" I don't care much which one, I liked "achieved" better.

sttts · 2020-10-07T08:58:17Z

pkg/operator/staticpod/controller/installer/installer_controller.go

 	}
 	// if we are progressing and no nodes have achieved that level, we should indicate
 	if numProgressing > 0 && counts[newStatus.LatestAvailableRevision] == 0 {
-		revisionStrings = append(revisionStrings, fmt.Sprintf("%d nodes have achieved new revision %d", 0, newStatus.LatestAvailableRevision))
+		revisionStrings = append(revisionStrings, fmt.Sprintf("none of the nodes have achieved new revision %d", newStatus.LatestAvailableRevision))


all these strings are a lot of text. Shouldn't we rather try to be more brief?

some nodes are on old revisions (34, 35), 1 is on latest.

yeah, I think it will read better than 3 semicolons

sttts · 2020-10-07T08:59:26Z

pkg/operator/staticpod/controller/installer/installer_controller.go

 	}
 	revisionDescription := strings.Join(revisionStrings, "; ")

 	if numAvailable > 0 {
+		message := fmt.Sprintf("%d nodes are active; %s", numAvailable, revisionDescription)


what does active mean?

good question, I think "active" means "available" which means "they achieved the desired levels and are running" ?

sttts · 2020-10-08T09:55:52Z

pkg/operator/staticpod/controller/installer/installer_controller.go

+			latestCount++
+		}
+		if c != 0 {
+			availableCount++


why does this mean anything about availability?

in the existing code "numAvailable" is determined by CurrentRevision!=0 ... So i guess CurrentRevision==0 means the node is not available or not actively making progress? i'm a bit confused here as well.

this has nothing to do with availability. It only says that this revision was the last that got ready eventually.

lastReadyRevision ?

call it nonZeroCount

lastReadyRevision

It's a count.

sttts · 2020-10-08T09:59:18Z

pkg/operator/staticpod/controller/installer/installer_controller.go

+
+	if !currentRevisions.Has(r.latestRevision) {
+		if r.progressing {
+			return fmt.Sprintf("%snodes progressing towards revision %d", notAvailable, r.latestRevision)


am confused about notAvailable (e.g. none of the nodes are available) to be plugged into the first %s.

i tried to incorporate that into message, so we don't lose it but also get rid of the ; .... I think it will read as "none of the nodes are available and nodes progressing towards revision N"

!currentRevisions.Has(r.latestRevision) does not match message. Better check that there is at least one node that is not at latestRevision.

oh. you mean "all nodes progressing ..."

sttts · 2020-11-30T10:07:30Z

pkg/operator/staticpod/controller/installer/installer_controller.go

@@ -422,15 +422,77 @@ func setNodeStatusFn(status *operatorv1.NodeStatus) v1helpers.UpdateStaticPodSta
 	}
 }

+type revisionDescriptionPrinter struct {


Why is this an object in the first place and not just a simple func?

sttts · 2020-11-30T10:29:54Z

pkg/operator/staticpod/controller/installer/installer_controller.go

+		}
+	}
+
+	notAvailable := fmt.Sprintf("%d %s are not available and ", availableCount, pluralizedNode(availableCount))


cannot follow. This must be len(nodes)-availableCount, no?

And better turn the wording into: %d %s have never been available and

sttts · 2020-11-30T10:30:08Z

pkg/operator/staticpod/controller/installer/installer_controller.go

+		notAvailable = ""
+	}
+	if availableCount == 0 {
+		return "none of the nodes are available"


none of the nodes was ever available.

sttts · 2020-11-30T10:34:16Z

pkg/operator/staticpod/controller/installer/installer_controller.go

+	}
+	if currentRevisions.Has(r.latestRevision) {
+		oldRevisionStringList := []string{}
+		for _, i := range currentRevisions.Difference(sets.NewInt32(r.latestRevision)).List() {


loop over currentRevisions and exclude latestRevision. No need for set arithmetic.

openshift-bot · 2021-02-28T13:30:39Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2021-03-30T15:23:23Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2021-04-29T18:56:40Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2021-04-29T18:56:46Z

@mfojtik: PR needs rebase.

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.

openshift-ci-robot · 2021-04-29T18:56:54Z

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.

openshift-ci-robot requested review from smarterclayton and sttts October 7, 2020 08:25

sttts reviewed Oct 7, 2020

View reviewed changes

staticpod: improve operator messages

e60740e

mfojtik force-pushed the improve-messages branch from 257962a to e60740e Compare October 8, 2020 09:29

sttts reviewed Oct 8, 2020

View reviewed changes

sttts reviewed Nov 30, 2020

View reviewed changes

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 28, 2021

openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 30, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2021

openshift-ci-robot closed this Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

staticpod: improve operator messages #913

staticpod: improve operator messages #913

mfojtik commented Oct 7, 2020

openshift-ci-robot commented Oct 7, 2020

sttts Oct 7, 2020

mfojtik Oct 7, 2020

sttts Oct 7, 2020

mfojtik Oct 7, 2020

sttts Oct 7, 2020

mfojtik Oct 7, 2020

sttts Oct 8, 2020

mfojtik Oct 8, 2020

sttts Nov 30, 2020

mfojtik Nov 30, 2020

sttts Nov 30, 2020

sttts Nov 30, 2020

sttts Oct 8, 2020

mfojtik Oct 8, 2020

sttts Nov 30, 2020 •

edited

sttts Nov 30, 2020

sttts Nov 30, 2020

sttts Nov 30, 2020

sttts Nov 30, 2020

sttts Nov 30, 2020

sttts Nov 30, 2020

openshift-bot commented Feb 28, 2021

openshift-bot commented Mar 30, 2021

openshift-bot commented Apr 29, 2021

openshift-ci bot commented Apr 29, 2021

openshift-ci-robot commented Apr 29, 2021

staticpod: improve operator messages #913

staticpod: improve operator messages #913

Conversation

mfojtik commented Oct 7, 2020

openshift-ci-robot commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts Nov 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Feb 28, 2021

openshift-bot commented Mar 30, 2021

openshift-bot commented Apr 29, 2021

openshift-ci bot commented Apr 29, 2021

openshift-ci-robot commented Apr 29, 2021

sttts Nov 30, 2020 •

edited