Skip to content

Equalise service stop or failures#637

Merged
oglok merged 5 commits into
openshift:mainfrom
mangelajo:eq-services
Mar 31, 2022
Merged

Equalise service stop or failures#637
oglok merged 5 commits into
openshift:mainfrom
mangelajo:eq-services

Conversation

@mangelajo

@mangelajo mangelajo commented Mar 25, 2022

Copy link
Copy Markdown
Contributor

This PR handles service stop/failure to make them all behave equally, and also to be able
to be stopped on request, please see individual commits.

  • Make kube-proxy Service stoppable
  • Exit on kustomization failure
  • Make oauth-api-server stoppable and return errors
  • Make ocp-api-server stoppable by manager
  • Let the OpenshiftControllerManager service to be stopped

@mangelajo mangelajo changed the title eq services Equalise service stop or failures Mar 25, 2022
@mangelajo mangelajo added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2022
@mangelajo mangelajo removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2022
mangelajo and others added 2 commits March 25, 2022 16:44
The kube-proxy k8s code does not support an stop channel, but
at least we can accept cancel on the context and move on, so
MicroShift can be stopped properly when necessary.

Related-Issue: openshift#556

Signed-off-by: Miguel Angel Ajo <majopela@redhat.com>
This normalizes the kustomization service with the other services,
and makes kustomization issues evident to the deployer.

Signed-off-by: Miguel Angel Ajo Pelayo <miguelangel@ajo.es>
@mangelajo

Copy link
Copy Markdown
Contributor Author

/retest

@mangelajo mangelajo requested review from fzdarsky and oglok March 28, 2022 11:37

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why create and pass in a new stop channel here, shouldn't this be ctx.Done()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same reason, this api server does not receive any context anywhere,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't fully understood your comment, moving to ctx.Done() which I remember doing and not working (channel directionality(, but it works.

Comment thread pkg/controllers/openshift-apiserver.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why create and pass in a new stop channel here, shouldn't this be ctx.Done()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As much as I looked I didn0t find the openshift-apiserver receiving the context in any form, so we can't cancel it that way. They use a stop channel. Hopefully this would change in a future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's why I wait for the context, outside, and propagate down to the stop channel.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As we talked OOB I'm extracting the stop channel with ctx.Done() for simpler code.

@copejon

copejon commented Mar 28, 2022

Copy link
Copy Markdown
Contributor

/retest

Signed-off-by: Miguel Angel Ajo <majopela@redhat.com>
Signed-off-by: Miguel Angel Ajo <majopela@redhat.com>
Signed-off-by: Miguel Angel Ajo <majopela@redhat.com>
@mangelajo

Copy link
Copy Markdown
Contributor Author

This one should be good now.

@mangelajo mangelajo requested a review from fzdarsky March 29, 2022 10:32
@oglok

oglok commented Mar 29, 2022

Copy link
Copy Markdown
Contributor

/retest

@oglok

oglok commented Mar 30, 2022

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2022
@openshift-ci

openshift-ci Bot commented Mar 30, 2022

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oglok

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 Mar 30, 2022
@oglok oglok merged commit 27767d9 into openshift:main Mar 31, 2022
@mangelajo mangelajo mentioned this pull request Jun 10, 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.

4 participants