-
Notifications
You must be signed in to change notification settings - Fork 9
Use ContainerPlatform.DEFAULT for selector label generation in KubeMirrorStrategy #953
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: munishchouhan <hrma017@gmail.com>
| // docker auth json file | ||
| final Path configFile = request.authJson ? request.workDir.resolve('config.json') : null | ||
| final selector = getSelectorLabel(request.platform, nodeSelectorMap) | ||
| final selector = getSelectorLabel(ContainerPlatform.DEFAULT, nodeSelectorMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would not be better request.platform ?: ContainerPlatform.DEFAULT ?
Also worth adding a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have hard-coded null, but a yes condition will be good for future changes.
Sure, I will add the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this better now. In reality the request.platform is not related with the platform used to run the container.
The request.platform is used to determine the container platform to be copied, it used null with the meaning all.
The selector determine the platform to be used to run copy job container, however this container is multi-arch i.e. a ARM64 container could copy a AMD64 container.
Ideally this container could run both on AMD64 or ARM64, however I think the deployment env requires at least to be chosen.
@gavinelder what's your take here, considering this job could run on both arch, could the selector be omitted or there's a selected that would allow to run on both ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a couple of options here based on the behaviour we want.
Node pools can have more than one label so if you're limited to specifying only one label on the deployment by using the nodeSelector resource we can add an additional label to both the AMD64 / ARM nodes allowing pods to be scheduled on both.
The more flexible but difficult task to implement is to support NodeAffinity rules which work as a node selector but can have more complex logic.
For example.
apiVersion: batch/v1
kind: Job
metadata:
name: wave-scan
spec:
template:
spec:
affinity:
nodeAffinity:
requiredDuringSchedulingIgnoredDuringExecution:
nodeSelectorTerms:
- matchExpressions:
- key: service
operator: In
values:
- wave-build-amd
- wave-build-arm
containers:
- name: job-container
image: my-image
restartPolicy: Never
Which would change the interface from a string to a list of strings on the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency I'd do seqera.io/wave-build-any=true instead of seqera.io/wave-supplemental=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@munishchouhan the keys need to be unique as such
'service=wave-build'
'service=wave-build-arm64'
Would not be possible.
seqera.io/wave-build-any=true
It is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but if we donot set any nodeselector for a pod where we want it to use seqera.io/wave-build-any=true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another approach #954
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please let me know which approach we want to take
- add node selector in mirror pod
- keep it null
Signed-off-by: munishchouhan <hrma017@gmail.com>
This PR will fix #952