Skip to content

OTA-1174: upgrade status: Show resources in --details=health - #1737

Merged
openshift-merge-bot[bot] merged 3 commits into
openshift:masterfrom
petr-muller:ota-1174-details-node-insights
Apr 24, 2024
Merged

OTA-1174: upgrade status: Show resources in --details=health#1737
openshift-merge-bot[bot] merged 3 commits into
openshift:masterfrom
petr-muller:ota-1174-details-node-insights

Conversation

@petr-muller

@petr-muller petr-muller commented Apr 19, 2024

Copy link
Copy Markdown
Member

Showing resources was a prerequisite to coalescing related insights I tried in #1690 where instead of showing an insight about each individual node, I wanted to display a single insight that would say "7 nodes in pool worker are degraded while draining: failed to..." and have a list of nodes affected by the one condition. I am not happy about my implementation attempt so I am not submitting that change, so that we can proceed and close OTA-1174. I will file a dedicated card for coalescing related insights instead.


upgrade status --detailed=health: show involved resources

The insight can contain data about involved resource (of various, and even mixed kinds). The detailed output can display this data.


upgrade status: ellipsize (duplicate) long names in messages

The node section is a table where node name is a dedicated column, so when the message mentions the name, it creates visual duplicate clutter, especially if the name is long and contains arbitrary string segments.

Ellipsize (replace with a placeholder) names longer than 7 characters from the messages.


upgrade status: hint at using --details=health

@openshift-ci-robot

openshift-ci-robot commented Apr 19, 2024

Copy link
Copy Markdown

@petr-muller: This pull request references OTA-1174 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

Details

In response to this:

Showing resources was a prerequisite to coalescing related insights I tried in #1690 where instead of showing an insight about each individual node, I wanted to display a single insight that would say "7 nodes in pool worker are degraded while draining: failed to..." and have a list of nodes affected by the one condition. I am not happy about my implementation attempt so I am not submitting that change, so that we can proceed and close OTA-1174. I will file a dedicated card for coalescing related insights instead.


upgrade status --detailed=health: show involved resources

The insight can contain data about involved resource (of various, and even mixed kinds). The detailed output can display this data.


upgrade status: ellipsize (duplicate) long names in messages

The node section is a table where node name is a dedicated column, so when the message mentions the name, it creates visual duplicate clutter, especially if the name is long and contains arbitrary string segments.

Ellipsize (replace with a placeholder) names longer than 7 characters from the messages.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 19, 2024
@openshift-ci-robot

openshift-ci-robot commented Apr 19, 2024

Copy link
Copy Markdown

@petr-muller: This pull request references OTA-1174 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

Details

In response to this:

Showing resources was a prerequisite to coalescing related insights I tried in #1690 where instead of showing an insight about each individual node, I wanted to display a single insight that would say "7 nodes in pool worker are degraded while draining: failed to..." and have a list of nodes affected by the one condition. I am not happy about my implementation attempt so I am not submitting that change, so that we can proceed and close OTA-1174. I will file a dedicated card for coalescing related insights instead.


upgrade status --detailed=health: show involved resources

The insight can contain data about involved resource (of various, and even mixed kinds). The detailed output can display this data.


upgrade status: ellipsize (duplicate) long names in messages

The node section is a table where node name is a dedicated column, so when the message mentions the name, it creates visual duplicate clutter, especially if the name is long and contains arbitrary string segments.

Ellipsize (replace with a placeholder) names longer than 7 characters from the messages.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci
openshift-ci Bot requested review from deads2k and soltysh April 19, 2024 16:05
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2024
@openshift-ci-robot

openshift-ci-robot commented Apr 19, 2024

Copy link
Copy Markdown

@petr-muller: This pull request references OTA-1174 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

Details

In response to this:

Showing resources was a prerequisite to coalescing related insights I tried in #1690 where instead of showing an insight about each individual node, I wanted to display a single insight that would say "7 nodes in pool worker are degraded while draining: failed to..." and have a list of nodes affected by the one condition. I am not happy about my implementation attempt so I am not submitting that change, so that we can proceed and close OTA-1174. I will file a dedicated card for coalescing related insights instead.


upgrade status --detailed=health: show involved resources

The insight can contain data about involved resource (of various, and even mixed kinds). The detailed output can display this data.


upgrade status: ellipsize (duplicate) long names in messages

The node section is a table where node name is a dedicated column, so when the message mentions the name, it creates visual duplicate clutter, especially if the name is long and contains arbitrary string segments.

Ellipsize (replace with a placeholder) names longer than 7 characters from the messages.


upgrade status: hint at using --details=health

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 openshift-eng/jira-lifecycle-plugin repository.

@petr-muller

Copy link
Copy Markdown
Member Author

/retest


resourceKindPad int
// resourceKinds contains keys of resources map
resourceKinds []string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we include the group too? Sometimes kind is not sufficient to be unique, e.g. in a TechPreviewNoUpgrade cluster:

$ oc api-resources | grep 'NAME\|^machines '
NAME                                  SHORTNAMES                                                                             APIVERSION                                      NAMESPACED   KIND
machines                              ma                                                                                     cluster.x-k8s.io/v1beta1                        true         Machine
machines                                                                                                                     machine.openshift.io/v1beta1                    true         Machine

And folks need to use oc get machines.machine.openshift.io if they want the machine-API type, because oc get machines will give them the cluster-API type.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

/hold

I'll check if I can address that easily

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, adding groups is a good idea.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done in 733af23

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Groups added:

/hold cancel

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2024
Comment thread pkg/cli/admin/upgrade/status/examples/4.14.1-degraded.output Outdated
Comment thread pkg/cli/admin/upgrade/status/examples/4.14.1-paused-worker-pool.output Outdated

resourceKindPad int
// resourceKinds contains keys of resources map
resourceKinds []string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, adding groups is a good idea.

@petr-muller
petr-muller force-pushed the ota-1174-details-node-insights branch from d72f24d to 190eddb Compare April 23, 2024 13:14
@DavidHurta

Copy link
Copy Markdown
Contributor

/lgtm
The new changes look good to me. The comments were addressed.

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2024
@wking

wking commented Apr 23, 2024

Copy link
Copy Markdown
Member

I dunno what's behind No rule to make target 'build', but seems unlikely to be related to this pull.

/retest-required

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2024
@petr-muller

Copy link
Copy Markdown
Member Author

I blame @soltysh #1652

@openshift-ci-robot

Copy link
Copy Markdown

/retest-required

Remaining retests: 0 against base HEAD 057cfc5 and 2 for PR HEAD 190eddb in total

@LalatenduMohanty LalatenduMohanty left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@petr-muller

Copy link
Copy Markdown
Member Author

/retest

@petr-muller

Copy link
Copy Markdown
Member Author

/retest

Maybe the failed build was cached, retrying after deleting

@petr-muller

Copy link
Copy Markdown
Member Author

/retest

One more shot after deleting all builds

@petr-muller

Copy link
Copy Markdown
Member Author

/retest

One more shot after openshift/release#51253 merged?

@petr-muller

Copy link
Copy Markdown
Member Author

Looks good now

INFO[2024-04-23T17:27:46Z] Image ci-op-14550stc/pipeline:tools created   for-build=tools
INFO[2024-04-23T17:27:46Z] Tagging tools into stable    

@openshift-ci-robot

Copy link
Copy Markdown

/retest-required

Remaining retests: 0 against base HEAD 8a664ae and 1 for PR HEAD 190eddb in total

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2024
The insight can contain data about involved resource (of various, and even mixed kinds). The detailed output can display this data.
The node section is a table where node name is a dedicated column, so when the message mentions the name, it creates visual duplicate clutter, especially if the name is long and contains arbitrary string segments.

Ellipsize (replace with a placeholder) names longer than 7 characters from the messages.
@petr-muller
petr-muller force-pushed the ota-1174-details-node-insights branch from 190eddb to e945000 Compare April 24, 2024 10:54
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2024
@petr-muller
petr-muller force-pushed the ota-1174-details-node-insights branch from e945000 to 385b964 Compare April 24, 2024 11:26
@petr-muller

Copy link
Copy Markdown
Member Author

Let's try a comment

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2024
@petr-muller

Copy link
Copy Markdown
Member Author

Good boy
/woof

@openshift-ci

openshift-ci Bot commented Apr 24, 2024

Copy link
Copy Markdown
Contributor

@petr-muller: dog image

Details

In response to this:

Good boy
/woof

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.

@LalatenduMohanty

Copy link
Copy Markdown
Member

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2024
@openshift-ci

openshift-ci Bot commented Apr 24, 2024

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Davoska, LalatenduMohanty, petr-muller

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-merge-bot
openshift-merge-bot Bot merged commit a08ffc9 into openshift:master Apr 24, 2024
@petr-muller
petr-muller deleted the ota-1174-details-node-insights branch April 24, 2024 15:28
@openshift-bot

Copy link
Copy Markdown
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-tools-container-v4.17.0-202404241149.p0.ga08ffc9.assembly.stream.el9 for distgit ose-tools.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants