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

Don't annotate nodes with NotManagedByMCM annotation #774

Open
himanshu-kun opened this issue Feb 1, 2023 · 7 comments
Open

Don't annotate nodes with NotManagedByMCM annotation #774

himanshu-kun opened this issue Feb 1, 2023 · 7 comments
Labels
area/usability Usability related kind/bug Bug lifecycle/stale Nobody worked on this for 6 months (will further age) needs/planning Needs (more) planning with other MCM maintainers priority/4 Priority (lower number equals higher priority)

Comments

@himanshu-kun
Copy link
Contributor

How to categorize this issue?

/area usability
/kind enhancement
/priority 3

What would you like to be added:
Currently any node which is added to the cluster and is not managed by MCM is annotated with node.machine.sapcloud.io/notManagedByMCM": "1"
This is a problem when there are two MCM's acting on the same target cluster from different control namespaces. This is usually the scenario in the IT we perform today.
In such a case because one MCM couldn't see the machine objects for other MCM which is in another namespace, it marks the node of the other MCM with this annotation.
And this way all the machines end up getting marked with the annotation.

Why is this needed:

  • To not mark every node with the annotation in case multiple MCM's are operating on the cluster
  • To enable customers to run another MCM deployment from their target cluster itself without being annotated.
@himanshu-kun himanshu-kun added the kind/enhancement Enhancement, improvement, extension label Feb 1, 2023
@gardener-robot gardener-robot added area/usability Usability related priority/3 Priority (lower number equals higher priority) labels Feb 1, 2023
@himanshu-kun
Copy link
Contributor Author

himanshu-kun commented Feb 1, 2023

Proposed solution

We can instead annotate the nodes which are managed by MCM with the annotation node.machine.sapcloud.io/managedByMCM: <place-holder>

<place-holder> needs to be unique for every MC which is running .
One solution is to use pod-name of the MCM pod where the MC is running.

  • but this has the drawback of not being able to name for the case when MC is running locally as a process.

Another solution is to use process-id

  • this has the issue of change in process-id name with a crash of the process.

A mix of the two solutions could be used as well.

cc @unmarshall @elankath for more comments

@unmarshall
Copy link
Contributor

but this has the drawback of not being able to name for the case when MC is running locally as a process.

You can pass a command line flag. If it detects that it is running in a K8S environment then it will default to pod-name. If it detects that it is not running in a pod then it will fail if the command line flag (name) is not provided.

@elankath
Copy link
Contributor

elankath commented Feb 1, 2023

but this has the drawback of not being able to name for the case when MC is running locally as a process.

For local cases, one can just fallback to the local host name.

@himanshu-kun
Copy link
Contributor Author

but this has the drawback of not being able to name for the case when MC is running locally as a process.

You can pass a command line flag. If it detects that it is running in a K8S environment then it will default to pod-name. If it detects that it is not running in a pod then it will fail if the command line flag (name) is not provided.

panicing for not providing the hostname ? I think falling back to local host name , and printing log for it , should be good enough.

@unmarshall
Copy link
Contributor

Yes hostname should serve as long as you are going to start a single process locally. If you wish to start more than one process locally then that will again not work.

panicing for not providing the hostname ?

Well its like any other required parameter that you pass to the command line. How is this going to be any different?

@himanshu-kun
Copy link
Contributor Author

Yes multiple case would be a problem, then making it required in local case is the only way to go.

@himanshu-kun himanshu-kun added priority/4 Priority (lower number equals higher priority) kind/bug Bug needs/planning Needs (more) planning with other MCM maintainers and removed priority/3 Priority (lower number equals higher priority) kind/enhancement Enhancement, improvement, extension labels Feb 16, 2023
@himanshu-kun
Copy link
Contributor Author

a solution proposed in issue #718 could also be used.

@gardener-robot gardener-robot added the lifecycle/stale Nobody worked on this for 6 months (will further age) label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/usability Usability related kind/bug Bug lifecycle/stale Nobody worked on this for 6 months (will further age) needs/planning Needs (more) planning with other MCM maintainers priority/4 Priority (lower number equals higher priority)
Projects
None yet
Development

No branches or pull requests

4 participants