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

sysdump feature detection: Don't depend on Cilium version #2032

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

michi-covalent
Copy link
Contributor

Detecting Cilium version can get arbitrary complicated. Let's not even try during sysdump feature detection since sysdump doesn't depend on any version-specific features yet.

This commit separates ExtractFromConfigMap into two functions:

  • ExtractFromConfigMap to detect version-independent features.
  • ExtractFromVersionedConfigMap to detect version-specific features.

Then sysdump uses ExtractFromConfigMap to extract enabled features it needs to know about without having to detect Cilium version. Problem solved, at least for now...

Fixes: #2029

Detecting Cilium version can get arbitrary complicated. Let's not even
try during sysdump feature detection since sysdump doesn't depend on any
version-specific features yet.

This commit separates ExtractFromConfigMap into two functions:

- ExtractFromConfigMap to detect version-independent features.
- ExtractFromVersionedConfigMap to detect version-specific features.

Then sysdump uses ExtractFromConfigMap to extract enabled features it
needs to know about without having to detect Cilium version. Problem
solved, at least for now...

Fixes: #2029

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@michi-covalent michi-covalent temporarily deployed to ci October 11, 2023 21:56 — with GitHub Actions Inactive
@michi-covalent michi-covalent marked this pull request as ready for review October 11, 2023 22:44
@michi-covalent michi-covalent requested review from a team as code owners October 11, 2023 22:44
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

Seems reasonable, but why does this actually fix the mentioned issue?

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 12, 2023
@michi-covalent
Copy link
Contributor Author

Seems reasonable, but why does this actually fix the mentioned issue?

oh yeah so the original issue was caused by this line:

			ciliumVersion, err := c.Client.GetCiliumVersion(context.Background(), c.CiliumPods[0])

GetCiliumVersion goes inside a cilium pod and run cilium version to get the cilium version, so it fails if cilium is not running 🙀

cilium-cli/k8s/client.go

Lines 967 to 988 in 7bb30e6

// GetCiliumVersion returns a semver.Version representing the version of cilium
// running in the cilium-agent pod
func (c *Client) GetCiliumVersion(ctx context.Context, p *corev1.Pod) (*semver.Version, error) {
o, _, err := c.ExecInPodWithStderr(
ctx,
p.Namespace,
p.Name,
defaults.AgentContainerName,
[]string{"cilium", "version", "-o", "jsonpath={$.Daemon.Version}"},
)
if err != nil {
return nil, fmt.Errorf("unable to fetch cilium version on pod %q: %w", p.Name, err)
}
v, _, _ := strings.Cut(strings.TrimSpace(o.String()), "-") // strips proprietary -releaseX suffix
podVersion, err := semver.Parse(v)
if err != nil {
return nil, fmt.Errorf("unable to parse cilium version on pod %q: %w", p.Name, err)
}
return &podVersion, nil
}

@michi-covalent michi-covalent merged commit 9f4c665 into main Oct 12, 2023
@michi-covalent michi-covalent deleted the pr/michi/sysdump-struggle branch October 12, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sysdump fails if one agent is unhealthy
3 participants