Skip to content

[release-4.10] Self-hibernate on OCP 4.11#874

Merged
openshift-merge-robot merged 6 commits intoopenshift-kni:release-4.10from
MarSik:feat-411-hibernate
Apr 12, 2022
Merged

[release-4.10] Self-hibernate on OCP 4.11#874
openshift-merge-robot merged 6 commits intoopenshift-kni:release-4.10from
MarSik:feat-411-hibernate

Conversation

@MarSik
Copy link
Member

@MarSik MarSik commented Apr 4, 2022

The functionality will be provided by NTO and PAO must
not interfere with it in case someone installs this
version on OCP 4.11.

See the following:

openshift/cluster-node-tuning-operator#322

The functionality will be provided by NTO and PAO must
not interfere with it  in case someone installs this
version on OCP 4.11.

See the following:

openshift/cluster-node-tuning-operator#322
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 4, 2022

@MarSik: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

WIP: Self-hibernate on OCP 4.11

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 openshift-ci bot requested review from jlojosnegros and yanirq April 4, 2022 08:13
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 4, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MarSik

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2022
@MarSik MarSik requested a review from cynepco3hahue April 4, 2022 08:13
@coveralls
Copy link

coveralls commented Apr 4, 2022

Pull Request Test Coverage Report for Build 2403

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 60.703%

Totals Coverage Status
Change from base Build 2389: 0.0%
Covered Lines: 1588
Relevant Lines: 2616

💛 - Coveralls

@cynepco3hahue
Copy link
Contributor

mm do we really want to do it? It will initiate the pod restart every time.

@yanirq
Copy link
Member

yanirq commented Apr 4, 2022

mm do we really want to do it? It will initiate the pod restart every time.

maybe we could just add a flag and avoid starting the manager?

if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {

@MarSik
Copy link
Member Author

MarSik commented Apr 4, 2022

mm do we really want to do it? It will initiate the pod restart every time.

Why should it? Liveness probe? I am specifically not killing the process, just hibernating.

And yes, we got a warning that some automated management systems might reinstall PAO 4.10 once NTO removes it in 4.11. It is of course a configuration bug on their side, however this looks like a rather cheap way to prevent such mistakes.

@MarSik
Copy link
Member Author

MarSik commented Apr 4, 2022

mm do we really want to do it? It will initiate the pod restart every time.

maybe we could just add a flag and avoid starting the manager?

if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {

mm do we really want to do it? It will initiate the pod restart every time.

maybe we could just add a flag and avoid starting the manager?

if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {

Well, that could probably work, it still needs to sleep though. Ending the process would cause a restart.

main.go Outdated

for {
select {
case sig := <-sigs:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do not need select here, we should just have a blocking channel here

if sig := <-sigs; sig == syscall.SIGTERM {
    signal.Stop(sigs)
    klog.Info("SIGTERM caught")
    break
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Well.. yeah, that would work as well. Isn't pretty much the same though? Easy to change anyway..

main.go Outdated
// Get OCP version objects (there should be just one)
clusterversionlist := v1.ClusterVersionList{}
ctx := context.Background()
err = k8sclient.List(ctx, &clusterversionlist)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: some syntax sugar

if err := k8sclient.List(context.TODO(), &clusterversionlist); err != nil {
    ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Line too long :D But yeah, I can do that.

@cynepco3hahue
Copy link
Contributor

Our CI is already running on top of 4.11 so if you do not filter CI cluster it will fail.

@yanirq
Copy link
Member

yanirq commented Apr 11, 2022

@MarSik please mark this PR as [release 4.10]

@MarSik MarSik changed the title WIP: Self-hibernate on OCP 4.11 [release-4.10] Self-hibernate on OCP 4.11 Apr 11, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 11, 2022

@MarSik: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

[release-4.10] Self-hibernate on OCP 4.11

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 openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2022
@MarSik
Copy link
Member Author

MarSik commented Apr 11, 2022

Seems to work as expected on a 4.11 nightly cluster

apiVersion: config.openshift.io/v1
kind: ClusterVersion
metadata:
  creationTimestamp: "2022-04-11T08:02:16Z"
  name: version
spec:
  clusterID: e2e66cc5-4e8e-4107-82b3-908772548632
status:
  ...
  desired:
      image: registry.build01.ci.openshift.org/ci-ln-bqn32bb/release@sha256:d5933447d968fd136a1f0fe3f8e7efaa3d90e4bc29a45b8666ac19842f2c3a57
      version: 4.11.0-0.nightly-2022-04-08-205307
I0411 08:30:33.358360 1 main.go:88] Operator Version: 4.5.0-0.beta.0-1022-g4493519d
I0411 08:30:33.358444 1 main.go:89] Git Commit: 4493519dc888ac3c7770c8023834527b7176268c
I0411 08:30:33.358451 1 main.go:90] Build Date: 2022-04-06T08:31:18+00:00
I0411 08:30:33.358456 1 main.go:91] Go Version: go1.17.8
I0411 08:30:33.358461 1 main.go:92] Go OS/Arch: linux/amd64
I0411 08:30:35.449609 1 request.go:665] Waited for 1.040324238s due to client-side throttling, not priority and fairness, request: GET:https://172.30.0.1:443/apis/ingress.operator.openshift.io/v1?timeout=32s
E0411 08:30:37.014209 1 main.go:160] This operator's functionality is provided by the Node Tuning Operator from now on. Hibernating.

@cynepco3hahue
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2022
@yanirq
Copy link
Member

yanirq commented Apr 12, 2022

/retest

@openshift-merge-robot openshift-merge-robot merged commit ea6a837 into openshift-kni:release-4.10 Apr 12, 2022
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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments