Skip to content

Conversation

@mburke5678
Copy link
Contributor

@mburke5678 mburke5678 commented Nov 6, 2019

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 6, 2019
@openshift-docs-preview-bot

The preview will be available shortly at:

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 7, 2019
@mburke5678
Copy link
Contributor Author

@adambkaplan @wking PTAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adambkaplan When would the use reset to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

nit: oc patch ... so you don't have to list the unrelated stuff like proxy settings?

Copy link
Member

Choose a reason for hiding this comment

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

nit: do we want to remove this for master? I thought this would work, and still be preferred, for 4.3+. But I'm unfamiliar with the registry internals.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wking this was from 3.x. OpenShift 4 has completely different namings for the deployment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking I was planning to merge this PR to master and cherrypick to 4.1 and 4.2 for now. Then merge https://github.com/openshift/openshift-docs/pull/17966/files to master when 4.3 is ready to go. The 4.3 PR has: $ oc rollout restart deployment/image-registry -n openshift-image-registry

@adambkaplan
Copy link
Contributor

@mburke5678 @wking I did the following to confirm that the following will work:

For 4.3 - oc rollout restart deployment image-registry -n openshift-image-registry

For 4.2 and 4.1 - oc patch deployment image-registry -n openshift-image-registry --type=merge --patch='{"spec":{"template":{"metadata":{"annotations":{"kubectl.kubernetes.io/restartedAt": "restartTimestamp"}}}}}' - note that the annotation value can be anything, we just prefer a UTC timestamp (it just needs to change with each restart)

@adambkaplan
Copy link
Contributor

That said, the reason this works cleanly is because of a bug: https://bugzilla.redhat.com/show_bug.cgi?id=1769927

These commands can force up to two rollouts on other operator-controlled deployments - one that adds the restart annotation, and a second rollout that the operator initiates to remove the annotation.

/cc @soltysh @sttts

Is the race that may trigger two rollouts something we should worry about (ex - for ocm or the apiservers)?

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 8, 2019
@mburke5678
Copy link
Contributor Author

mburke5678 commented Nov 8, 2019

@adambkaplan Is the oc rollout restartsubcommand in the oc yet? Maybe I have the wrong version.

$ oc version
Client Version: openshift-clients-4.3.0-201910250623-42-gc276ecb7
Server Version: 4.3.0-0.nightly-2019-11-08-094604
Kubernetes Version: v1.16.2
~ $ oc rollout restart
error: unknown command "restart"
See 'oc rollout -h' for help and examples

@adambkaplan
Copy link
Contributor

@tnozicka is this the correct command upstream? Do we need to file a BZ?

1 similar comment
@mburke5678
Copy link
Contributor Author

@tnozicka is this the correct command upstream? Do we need to file a BZ?

@tnozicka
Copy link
Contributor

yes, file a BZ to wire it

@tnozicka
Copy link
Contributor

assign the BZ to me, fyi the fix is here openshift/oc#169

@mburke5678 mburke5678 merged commit 7465c96 into openshift:master Nov 29, 2019
@mburke5678 mburke5678 deleted the BZ-1730148 branch November 29, 2019 18:42
@mburke5678
Copy link
Contributor Author

/cherrypick enterprise-4.1

@openshift-cherrypick-robot

@mburke5678: new pull request created: #18373

Details

In response to this:

/cherrypick enterprise-4.1

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.

@mburke5678
Copy link
Contributor Author

/cherrypick enterprise-4.2

@openshift-cherrypick-robot

@mburke5678: new pull request created: #18374

Details

In response to this:

/cherrypick enterprise-4.2

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.

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

Labels

branch/enterprise-4.1 branch/enterprise-4.2 size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants