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

Reevaluate the PodSpec equality factors #3103

Open
2 of 3 tasks
tenzen-y opened this issue Sep 19, 2024 · 4 comments
Open
2 of 3 tasks

Reevaluate the PodSpec equality factors #3103

tenzen-y opened this issue Sep 19, 2024 · 4 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@tenzen-y
Copy link
Member

tenzen-y commented Sep 19, 2024

What would you like to be added:
We want to reevaluate the PodSpec equality factors and use only the necessary factors (fields) to identify the PodSpec.

func comparePodTemplate(a, b *corev1.PodSpec, ignoreTolerations bool) bool {
if !ignoreTolerations && !equality.Semantic.DeepEqual(a.Tolerations, b.Tolerations) {
return false
}
if !equality.Semantic.DeepEqual(a.InitContainers, b.InitContainers) {
return false
}
return equality.Semantic.DeepEqual(a.Containers, b.Containers)
}

Why is this needed:
The current approach sometimes causes unexpected update detection as we discussed in #3090.
So, it would be better to adopt the selective equality evaluation as opposed to the current nonselective equality.

Before we modify the actual equality function, we need to summarize the necessary fields for developers and users.

Completion requirements:

This enhancement requires the following artifacts:

  • Design doc
  • API change
  • Docs update

The artifacts should be linked in subsequent comments.

@tenzen-y tenzen-y added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 19, 2024
@tenzen-y
Copy link
Member Author

cc: @alculquicondor @dgrove-oss

@tenzen-y tenzen-y changed the title Reevaluate the PodSpec identify factors Reevaluate the PodSpec equality factors Sep 19, 2024
@alculquicondor
Copy link
Contributor

Interestingly, I think we are missing some important fields, such as pod overhead.

But from within the containers, we just care about requests.

@mimowo anything else in the context of TAS?

@alculquicondor
Copy link
Contributor

FYI: we need to be careful about pod group support, as we use the fields to build hashes. That list of fields will be harder to change. If we change the list and a pod group is running while there is a kueue upgrade, the new pods might be considered invalid.

@mimowo
Copy link
Contributor

mimowo commented Sep 23, 2024

From the perspective of TAS I don't see a need to change this yet, but let me check my understanding.

There are 2 levels here: PodSpec and Container, and the current approach for comparing is mixed (based on the quoted comparePodTemplate function):

  • selective at the level of PodSpec (only check Containers, InitContainers, and Tolerations), see the full list of fields here
  • non-selective at the lower level of containers (the full list of fields here)

So, the comment in code suggests to be less selective at the PodSpec level, while the issue here suggests to be more selective at the container level?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants