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

Scheduler jobs interceptions fixed #656

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Scheduler jobs interceptions fixed #656

merged 1 commit into from
Jan 2, 2024

Conversation

worryg0d
Copy link
Collaborator

Problem

There is method GetJobID for each k8s resource that needs a scheduler and the current implementation of the method uses the following patter: metadata.namespace/metadata.name/jobname. When there are two resources with different Kind, but the same namespaced names it causes intersections in the scheduler job, so we can not run a job for the second resource.

This PR fixes the problem by adding a Kind of resources for each GetJobID method.
New pattern: metadata.kind/metadata.namespace/metadata.name/jobname

@worryg0d worryg0d added the bug Something isn't working label Dec 22, 2023
@worryg0d worryg0d self-assigned this Dec 22, 2023
}

func (r *AWSEndpointServicePrincipal) GetJobID(job string) string {
return r.Namespace + "/" + r.Name + "/" + job
func (principal *AWSEndpointServicePrincipal) GetJobID(job string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename receiver principal to p

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -63,7 +63,7 @@ type AWSEncryptionKeyList struct {
}

func (aws *AWSEncryptionKey) GetJobID(jobName string) string {
return client.ObjectKeyFromObject(aws).String() + "/" + jobName
return aws.Kind + "/" + client.ObjectKeyFromObject(aws).String() + "/" + jobName
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just use UUID-Based Naming. It would be like aws.ObjectMeta.UID + "/" + jobName. You can even add client.ObjectKeyFromObject(aws).String() there. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The following solution allows us to easly watch in the operator logs for which resource job is created. However, we can also add UUID to the job name. What do others think about a pattern with UUID included?metadata.uuid/metadata.kind/metadata.namespace/metadata.name/jobid
It looks a bit big, but it provides more uniqueness for JobIDs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I discussed it with @ribaraka and decided to use metadata.kind/metadata.namespace/metadata.name/jobId because it is easier to manage jobs via the operator logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks also good

@testisnullus testisnullus merged commit a57e86c into main Jan 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants