ipv6 cluster creation issue in testsys #954
Conversation
zongzhidanielhu
force-pushed
the
ipfamily_ipv4_issue
branch
from
22a2204 to
d2830e5
Compare
zongzhidanielhu
changed the title
zongzhidanielhu
force-pushed
the
ipfamily_ipv4_issue
branch
from
226a7f9 to
908630a
Compare
|
LGTM. Can you fix the checks by following the error messages? thanks! |
… fix previously wrong ipFamily issue This is the body of the commit: this fix addressed the issue of when create clusters, the ipv6 cluster always has a ipFamily of ipv4 which is wrong. In this commit, a new way of creating cluster is added which is using created yaml file in a certain path. That way we can definite the ipFamily for whether it is ipv6 or ipv4.
zongzhidanielhu
force-pushed
the
ipfamily_ipv4_issue
branch
from
80db758 to
32eabc2
Compare
| @@ -116,6 +121,7 @@ impl AwsClients { | |||
| } | |||
| } | |||
|
|
|||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
What dead code are we allowing?
| } | ||
|
|
||
| fn create_yaml(cluster_name: &str, region: &str, version: &str) -> ProviderResult<()> { | ||
| let ip_family = if cluster_name.ends_with("ipv6") { |
There was a problem hiding this comment.
This would probably be better solved with an Enum since we know there are two valid values and only two valid values.
| @@ -129,6 +135,103 @@ enum ClusterConfig { | |||
| }, | |||
| } | |||
|
|
|||
| #[derive(Serialize)] | |||
| #[serde(rename_all = "camelCase")] | |||
| struct Config { | |||
There was a problem hiding this comment.
Can we get doc strings for all these structs? I think this is specifically an eksctl configuration struct, but a docstring would help. I think EksctlConfig might be a better name as well to signal we don't own this format, its intended to be used specifically as input to eksctl.
| #[derive(Serialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| struct KubernetesNetworkConfig { | ||
| ip_family: String, |
There was a problem hiding this comment.
If you made this type an Enum, you could use the type system to protect your values against unknown/invalid ones.
| }], | ||
| }; | ||
|
|
||
| let yaml = serde_yaml::to_string(&cluster).expect("Failed to serialize YAML"); |
There was a problem hiding this comment.
Can we use .context() here to stay consistent with the rest of the codebase on error handling?
| /// The default region for the cluster. | ||
| const DEFAULT_REGION: &str = "us-west-2"; | ||
| /// The default cluster version. | ||
| const DEFAULT_VERSION: &str = "1.24"; | ||
| const TEST_CLUSTER_CONFIG_PATH: &str = "/local/eksctl_config.yaml"; | ||
| const CLUSTER_CONFIG_PATH: &str = "/local/cluster_config.yaml"; |
There was a problem hiding this comment.
Why did you create your own cluster_config.yaml file here? Do we need both this and TEST_CLUSTER_CONFIG_PATH?
| @@ -194,53 +288,35 @@ impl ClusterConfig { | |||
| } | |||
|
|
|||
| /// Create a cluster with the given config. | |||
| #[allow(clippy::let_and_return)] | |||
There was a problem hiding this comment.
Why did you add this? I think we want to fix the code, not disable the lint.
| Self::Args { | ||
| cluster_name, | ||
| region, | ||
| version, | ||
| zones, | ||
| zones: _, |
There was a problem hiding this comment.
I don't follow the change to understand why you needed to change this? Are we no longer handling zones? Won't this break code where we have to be very clear which zone we are launching in when there is capacity only in a certain zone?
| @@ -144,15 +247,6 @@ impl ClusterConfig { | |||
| )?) | |||
| .context(Resources::Clear, "Unable to serialize eksctl config.")?; | |||
|
|
|||
There was a problem hiding this comment.
It seems like you just broke the ability to pass in a file for ClusterConfig? Can you explain why you removed this?
There was a problem hiding this comment.
I didn't change this part of the code, used to add some code here later on I recovery to it original state.
| }; | ||
|
|
||
| trace!("Calling eksctl create cluster with config file"); |
There was a problem hiding this comment.
nit: We probably want to indicate which configuration file we are calling with. This could be very confusing if its using the one you generate vs one passed in and this log line wouldn't help the user discern which one is happening.
Issue number:
ipv6 variants always have a ipFamily=ipv4
Description of changes:
changed the way of create cluster so that ipv6 has ipFamily=ipv6, ipv4 has ipFamily=ipv4
Testing done:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.