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.

Sign up for GitHub

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

diagnose: handle undetermined network plugins #1016

Merged
merged 2 commits into from
Dec 5, 2023
Merged

diagnose: handle undetermined network plugins #1016

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e686cd
diagnose: handle undetermined network plugins
skitt Nov 30, 2023
5de73dc
Merge branch 'devel' into diagnose-unknown-cni
tpantelis Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/diagnose/cni.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ func CNIConfig(clusterInfo *cluster.Info, _ string, status reporter.Interface) e
status.Start("Checking Submariner support for the CNI network plugin")
defer status.End()

if clusterInfo.Submariner.Status.NetworkPlugin == "" {
status.Warning("The network plugin hasn't been determined yet.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This implies it would be determined at some point? But shouldn't L85 below handle this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is determined later. Line 85 handles the case but produces a confusing message, indicating that the plugin isn’t supported when the reality is that Submariner doesn’t know what the plugin is yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's L80. L8(5|6) says "Submariner could not detect the CNI network plugin and is using (%q) plugin.". Isn't that the message we want? Perhaps we just need to move L85-88 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, yes. But that message would be inaccurate: if NetworkPlugin is empty in the status, Submariner isn’t using the generic plugin, it’s just not detected the plugin yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh right - this is for diagnose.

Copy link
Member

Choose a reason for hiding this comment

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

I feel the message The network plugin hasn't been determined yet could give an impression to the user that if they retry the diagnose command after a while, the CNI plugin could be determined by then.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sridhargaddam yes, that’s exactly what should happen. The situation handled here only happens if subctl diagnose is run before the plugin has been identified; once it has, diagnose will show the type (or “generic” if it isn’t known).

Copy link
Member

Choose a reason for hiding this comment

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

got it, thanks for the clarification.

return nil
}

isSupportedPlugin := false

for _, np := range supportedNetworkPlugins {
Expand Down
Loading