Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Remove .spec.network.mode; use a global --network-plugin flag instead #319

Merged
merged 7 commits into from
Aug 12, 2019

Conversation

luxas
Copy link
Contributor

@luxas luxas commented Aug 12, 2019

Fixes: #310
cc @twelho

@luxas luxas added this to the v0.5.0 milestone Aug 12, 2019
@luxas luxas added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 12, 2019
@luxas luxas force-pushed the instance-specific-networkmode branch from 0b76084 to 909d995 Compare August 12, 2019 10:13
@luxas luxas changed the title Instance specific networkmode Make network mode instance-specific and remove it from the API Aug 12, 2019
Copy link
Contributor

@twelho twelho left a comment

Choose a reason for hiding this comment

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

Great changes!
Some nits, otherwise LGTM 👍


// Convert_v1alpha1_VMNetworkSpec_To_ignite_VMNetworkSpec calls the autogenerated conversion function and possibly custom conversion logic
func Convert_v1alpha1_VMNetworkSpec_To_ignite_VMNetworkSpec(in *VMNetworkSpec, out *ignite.VMNetworkSpec, s conversion.Scope) error {
// .Spec.Network.Mode has been removed in v1alpha2. However, we won't persist that information anywhere in the new API, so
Copy link
Contributor

Choose a reason for hiding this comment

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

.Spec.Network.Mode -> .spec.network.mode, the wording on this comment is also a bit funky. Just something like "The network mode is not tracked in the new API, so there's no extra conversion logic" would be enough.

@@ -174,3 +174,10 @@ func Convert_v1alpha1_VMKernelSpec_To_ignite_VMKernelSpec(in *VMKernelSpec, out

return Convert_v1alpha1_OCIClaim_To_ignite_OCI(&in.OCIClaim, &out.OCI)
}

// Convert_v1alpha1_VMNetworkSpec_To_ignite_VMNetworkSpec calls the autogenerated conversion function and possibly custom conversion logic
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove "possibly"

}
}
if foundPlugin == nil {
return fmt.Errorf("Invalid network mode %q, must be one of %v", val, plugins)
Copy link
Contributor

Choose a reason for hiding this comment

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

Invalid -> invalid

for _, plugin := range plugins {
if plugin.String() == val {
foundPlugin = &plugin
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Perform *nf.value = plugin here and return instead of break?

return fmt.Errorf("Invalid network mode %q, must be one of %v", val, plugins)
}
*nf.value = *foundPlugin
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's a return in the loop, you can just return fmt.Errorf("invalid network mode %q, must be one of %v", val, plugins) here

}

func (nf *NetworkPluginFlag) Type() string {
return "network-mode"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this plugin, so the flag would be --network-plugin plugin instead of --network-plugin network-mode

var _ pflag.Value = &NetworkPluginFlag{}

func NetworkPluginVar(fs *pflag.FlagSet, ptr *network.PluginName) {
fs.Var(&NetworkPluginFlag{value: ptr}, "network-plugin", fmt.Sprintf("Networking mode to use. Available options are: %v", plugins))
Copy link
Contributor

Choose a reason for hiding this comment

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

"Network plugin to use..."

networkPlugin := providers.NetworkPlugins[vm.Spec.Network.Mode.String()]
log.Debugf("Removing the container with ID %q from the %s network", networkPlugin.Name(), containerID)
return networkPlugin.RemoveContainerNetwork(containerID)
log.Debugf("Removing the container with ID %q from the %s network", providers.NetworkPlugin.Name(), containerID)
Copy link
Contributor

Choose a reason for hiding this comment

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

%s network -> %q network for consistency

@luxas luxas force-pushed the instance-specific-networkmode branch from 909d995 to fb0114d Compare August 12, 2019 11:20

// Convert_v1alpha1_VMNetworkSpec_To_ignite_VMNetworkSpec calls the autogenerated conversion function and custom conversion logic
func Convert_v1alpha1_VMNetworkSpec_To_ignite_VMNetworkSpec(in *VMNetworkSpec, out *ignite.VMNetworkSpec, s conversion.Scope) error {
// .Spec.Network.Mode is not tracked in the v1alpha2 and newer, so there's no extra conversion logic
Copy link
Contributor

Choose a reason for hiding this comment

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

".spec.network.mode is not tracked in v1alpha2 and newer, so there's no extra conversion logic"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.Spec.Network.Mode is exactly as valid, even more IMO. .spec.network.mode is the JSON path

@luxas
Copy link
Contributor Author

luxas commented Aug 12, 2019

Review comments addressed, merging...

@luxas luxas merged commit a6f1a2d into weaveworks:master Aug 12, 2019
@luxas luxas changed the title Make network mode instance-specific and remove it from the API Remove .spec.network.mode; use a global --network-plugin flag instead Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove .spec.network.mode, make it instance-specific
2 participants