Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run elasticsearch operator on infra nodes #44

Closed
wants to merge 4 commits into from

Conversation

bastjan
Copy link
Contributor

@bastjan bastjan commented Apr 19, 2022

Checklist

  • PR contains a single logical change (to build a better changelog).
  • Update the documentation.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog.

@bastjan bastjan self-assigned this Apr 19, 2022
@bastjan bastjan force-pushed the operator-on-infra-nodes branch from 11c1300 to 205f9ac Compare April 19, 2022 15:09
@bastjan bastjan marked this pull request as ready for review April 19, 2022 15:22
@bastjan bastjan requested a review from simu April 19, 2022 15:22
Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

Not sure what, if anything is left from this PR, if we move the OLM operator node selector config into component openshift4-operators.

} + (import 'kibana-host.libsonnet')
}
+ (import 'kibana-host.libsonnet')
+ (import 'deployment-patch.libsonnet')
Copy link
Member

Choose a reason for hiding this comment

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

I think we should address scheduling of operator pods which are managed through component openshift4-operators in that component (https://github.com/appuio/component-openshift4-operators) and not in the components which make use of component openshift4-operators.

I'd suggest to simply set the openshift.io/node-selector: node-role.kubernetes.io/infra= annotation on the namespaces managed by that component.

Alternatively the component library of component openshift4-operators could support configuring deployment patches, but I don't think that's actually necessary in this case, since we'll always want all operators to run on the infra nodes, if there are any.

Copy link
Contributor Author

@bastjan bastjan Apr 19, 2022

Choose a reason for hiding this comment

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

That was my first instinct too. But i found the openshift.io/node-selector annotation already explicitly set to empty.

But I'm unsure if this is really needed...

Copy link
Member

@simu simu Apr 19, 2022

Choose a reason for hiding this comment

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

Looks like I copied that from the official instructions, cf. https://docs.openshift.com/container-platform/4.9/logging/cluster-logging-deploying.html#cluster-logging-deploy-cli_cluster-logging-deploying but I don't see how that's necessary unless some operators need to run on the control plane nodes.

I'd propose to go ahead and change the annotation in that component, since there's no real reason for the operator deployments themselves to run on specific nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 makes for a way simpler change

Copy link
Member

Choose a reason for hiding this comment

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

+1 makes for a way simpler change

IMO the even bigger upside is that we don't need to copy-paste this change to any component which installs an operator via the openshift4-operators component library.

@bastjan bastjan closed this Apr 19, 2022
@bastjan bastjan deleted the operator-on-infra-nodes branch April 19, 2022 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants