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

netdog: Persist current IP and add subcommand node-ip for retrieval #1502

Merged
merged 1 commit into from
Apr 21, 2021

Conversation

zmrow
Copy link
Contributor

@zmrow zmrow commented Apr 19, 2021

Issue number:
Related to #1458

Description of changes:

With this change, netdog will persist the node's current IP (read from
it's DHCP leases file) to /var/lib/netdog/current_ip.  It does this as
part of its "install" subcommand, which is run each time that wickedd
receives an update.  A new subcommand has also been added, which
retrieves the current IP from said file and returns it as JSON.  This
subcommand is intended for use as a settings generator.

This change is meant to support the VMWare variant #1451, specifically the kubernetes.node-ip setting and will receive further exercise with that PR.

Testing done:

  • Build and run an aws-dev variant to ensure that the file containing the IP exists with the correct data and the subcommand works:
bash-5.0# ls -lahZ /var/lib/netdog/current_ip 
-rw-r--r--. 1 root root system_u:object_r:lease_t:s0 11 Apr 19 20:15 /var/lib/netdog/current_ip

bash-5.0# cat /var/lib/netdog/current_ip 
10.0.60.220

bash-5.0# netdog node-ip
"10.0.60.220"
  • Build and run an aws-k8s-1.19 node, started sheltie and checked files. Node joins the cluster just fine and I can run pods on it.

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.

Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

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

Can you also verify that /etc/resolv.conf and /proc/sys/kernel/hostname are still set correctly?

@@ -2088,6 +2085,7 @@ dependencies = [
"rand 0.8.3",
"regex",
"serde",
"serde_json",
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect it'd work with just this change to Cargo.lock and not all the rest of it. Can you try that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a cargo update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't meant to be. I'm going to implement @bcressey 's ask above to avoid it.

Comment on lines 341 to 350
match parse_args(env::args())? {
(SubCommand::NodeIp, None) => node_ip()?,
(subcommand, Some(args)) => match subcommand {
SubCommand::Install => install(&args)?,
SubCommand::Remove => remove(&args)?,
SubCommand::NodeIp => {
usage_msg(format!("Subcommand 'node-ip' doesn't support arguments"))
}
},
(_, None) => usage_msg(format!("Subcommands 'install/remove' require arguments")),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this might be clearer overall, since it doesn't mix NodeIp in with the handling of Install and Remove:

Suggested change
match parse_args(env::args())? {
(SubCommand::NodeIp, None) => node_ip()?,
(subcommand, Some(args)) => match subcommand {
SubCommand::Install => install(&args)?,
SubCommand::Remove => remove(&args)?,
SubCommand::NodeIp => {
usage_msg(format!("Subcommand 'node-ip' doesn't support arguments"))
}
},
(_, None) => usage_msg(format!("Subcommands 'install/remove' require arguments")),
match parse_args(env::args())? {
(SubCommand::NodeIp, None) => node_ip()?,
(SubCommand::NodeIp, Some(_) => usage_msg(format!("Subcommand 'node-ip' doesn't support arguments")).
(subcommand, Some(args)) => match subcommand {
SubCommand::Install => install(&args)?,
SubCommand::Remove => remove(&args)?,
},
(_, None) => usage_msg(format!("Subcommands 'install/remove' require arguments")),

Copy link
Contributor

Choose a reason for hiding this comment

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

(_, None) => usage_msg(format!("Subcommands 'install/remove' require arguments"))

For this it might be better to use the subcommand parameter so you can add it to the format string.

Copy link
Contributor

@webern webern Apr 20, 2021

Choose a reason for hiding this comment

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

I suggest:

enum Subcommand {
    Install(Args),
    Remove(Args),
    NodeIp,
}

And

fn parse_args(args: env::Args) -> Result<SubCommand> {
   // ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually since, since this would require us to stop using serde_plain to 'deserialize' the subcommand string, it's probably not worth it.

Copy link
Contributor Author

@zmrow zmrow Apr 20, 2021

Choose a reason for hiding this comment

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

@bcressey - The compiler wasn't smart enough to do your suggestion, it still wanted an exhaustive match of the enum variants inside the second match. I opted to split out the options a bit further and just avoid a nested match altogether.

Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

Is persisting the IP to a file an optimization (i.e. for faster queries)? Or is the persistence needed across reboots?

@@ -2088,6 +2085,7 @@ dependencies = [
"rand 0.8.3",
"regex",
"serde",
"serde_json",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a cargo update?

@zmrow
Copy link
Contributor Author

zmrow commented Apr 20, 2021

@webern Persisting the IP to file means that we can use it as the static IP for the setting kubernetes-node-ip, specifically for VMWare (since we don't have the luxury of IMDS).

With this change, netdog will persist the node's current IP (read from
it's DHCP leases file) to /var/lib/netdog/current_ip.  It does this as
part of its "install" subcommand, which is run each time that wickedd
receives an update.  A new subcommand has also been added, which
retrieves the current IP from said file and returns it as JSON.  This
subcommand is intended for use as a settings generator.
@zmrow
Copy link
Contributor Author

zmrow commented Apr 20, 2021

^ Addressed @bcressey 's comments and verified that /etc/resolv.conf and /proc/sys/kernel/hostname are both still set correctly.

@zmrow zmrow merged commit f00f82e into bottlerocket-os:develop Apr 21, 2021
@zmrow zmrow deleted the netdog-node-ip branch April 21, 2021 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants