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

Generate network config on first and subsequent boots #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prakashsurya
Copy link
Contributor

This change modifies when cloud-init will generate the network config
for the system, such that it will now be generated for the first boot
(which it previously did) and all subsequent boots (which is new). This
way, if the system's network hardware changes (e.g. the MAC address
changes), the network configuration for the system will automatically
adapt on the next boot.

Also note that for a DE, this only applies when using the default
cloud-init generated network configuration. Currently, the cloud-init
config is only used prior to the DE administrator performing any manual
network configuration via the API or CLI. After manual network changes
are made, the DE will no longer use the cloud-init network config, at
which point this change will no longer have any affect.

This change modifies when cloud-init will generate the network config
for the system, such that it will now be generated for the first boot
(which it previously did) and all subsequent boots (which is new). This
way, if the system's network hardware changes (e.g. the MAC address
changes), the network configuration for the system will automatically
adapt on the next boot.

Also note that for a DE, this only applies when using the default
cloud-init generated network configuration. Currently, the cloud-init
config is only used prior to the DE administrator performing any manual
network configuration via the API or CLI. After manual network changes
are made, the DE will no longer use the cloud-init network config, at
which point this change will no longer have any affect.
@prakashsurya
Copy link
Contributor Author

git-ab-pre-push is here

@sebroy
Copy link

sebroy commented Aug 31, 2020

If the network configuration is later changed, do we correctly stop this auto-generation of configuration from happening at every boot?

@prakashsurya
Copy link
Contributor Author

@sebroy yes, that's what I was getting at in the description:

Currently, the cloud-init config is only used prior to the DE administrator performing any manual network configuration via the API or CLI. After manual network changes are made, the DE will no longer use the cloud-init network config, at which point this change will no longer have any affect.

After making changes to the network configuration via the DE, we disable the cloud-init network configuration, and rely on the DE software to manage the network configuration.

@sdimitro
Copy link

If the network configuration is later changed, do we correctly stop this auto-generation of configuration from happening at every boot?

I think that is the goal according to Prakash's description above:

Also note that for a DE, this only applies when using the default
cloud-init generated network configuration. Currently, the cloud-init
config is only used prior to the DE administrator performing any manual
network configuration via the API or CLI. After manual network changes
are made, the DE will no longer use the cloud-init network config, at
which point this change will no longer have any affect.

I think the Blackbox tests should cover this case (e.g. rebooting and the admin-applied configuration is still there). @prakashsurya could you verify that? if that's not the case at least having some manual testing output in the PR would be good.

@prakashsurya
Copy link
Contributor Author

Additionally, I should add, I don't think this change is strictly necessary. I plan to propose another change that removes the MAC address matching for both, the cloud-init network configuration and the DE network configuration. So, once we do that, we likely won't need to regenerate the network configuration, even for the cloud-init generated config.

@sebroy
Copy link

sebroy commented Aug 31, 2020

@sebroy yes, that's what I was getting at in the description:

Wow, I can't believe I missed that the 1st time. Sorry for the distraction.

@prakashsurya
Copy link
Contributor Author

I just verified that after making a network change via the DE, and then rebooting the system, the cloud-init reconfiguration on boot was not triggered (with this cloud-init change applied to the system); the DE configuration remained.

@pzakha
Copy link
Contributor

pzakha commented Aug 31, 2020

Additionally, I should add, I don't think this change is strictly necessary. I plan to propose another change that removes the MAC address matching for both, the cloud-init network configuration and the DE network configuration. So, once we do that, we likely won't need to regenerate the network configuration, even for the cloud-init generated config.

I'm curious to know why you've decided to go with this change now, rather than going for the change that would remove the MAC address matching.

@prakashsurya
Copy link
Contributor Author

rather than going for the change that would remove the MAC address matching.

I would not categorize the situation as going with this change, or removing of the MAC address matching. I still plan to open another PR that removes the MAC address matching. This change, and that future one, are independent.

I'm curious to know why you've decided to go with this change now

Even with the MAC address matching removal (which I haven't yet opened a PR for), I think we may want to adopt this change simply due to the fact that this is how cloud-init worked before, and this behaviour was "silently" changed without us realizing (in commit be9ecc1, as far as I can tell).

With that said, I don't think we need this change once we remove the MAC address matching, so I'd be OK not landing this, and only moving forward with the removal of the MAC maching, if that's what folks would prefer.

@pzakha
Copy link
Contributor

pzakha commented Aug 31, 2020

I could see arguments for going either way, so I mostly wanted to hear your opinion on that.

  • The point in favour would be: This change could alleviate some immediate issues that customers have due to a changing MAC address
  • The point against this: This change would create an inconsistent behaviour between engines with networking maintained by cloud-init and those maintained by Delphix (with changes done to the default network config). This would also mean that customers that suffer issues with the MAC address changing right now should be notified to not modify the default networking configuration.

So I'm fine either way.

@prakashsurya
Copy link
Contributor Author

This would also mean that customers that suffer issues with the MAC address changing right now should be notified to not modify the default networking configuration.

IMO, this is irrelevant. This proposed change is not intended to address our customer issues w.r.t. the MAC changing. I'll address that in another PR, that removes the MAC address matching from both the cloud-init configuration and our DE configuration (i.e. a change to cloud-init, and another change to the virtualization package).

I opened this PR simply because I had it ready and tested, not that I intended for this to address our customers' problems.

This change would create an inconsistent behaviour between engines with networking maintained by cloud-init and those maintained by Delphix (with changes done to the default network config).

I agree. Since we don't regenerate the DE generated network configuration on each boot, we may not want to regenerate the cloud-init generated network configuration on each boot either; to be consistent between the two.

@grwilson
Copy link
Contributor

Out of curiosity, is this something that should be upstreamed or is the upstream behavior what most people would expect?

@prakashsurya
Copy link
Contributor Author

Since upstream explicitly moved away from the old behaviour, which was to regenerate the network config on each boot, I don't think this is something to be upstreamed; I presume they moved away from regeneration on each boot for a reason.

And as I've mentioned, I think we could likely make do without this change, once I update the config to be resilient to MAC address changes.

@grwilson
Copy link
Contributor

Since upstream explicitly moved away from the old behaviour, which was to regenerate the network config on each boot, I don't think this is something to be upstreamed; I presume they moved away from regeneration on each boot for a reason.

Ah, so I wonder if it makes sense to make this change since it has limited value to our product and directly goes away from the behavior that upstream provides. I see the benefit of regenerating on every boot but I wonder if we're overlooking an issue that upstream deemed undesirable. Do you think the benefit of this change is worth taking on the technical debt of caring this change forever?

@prakashsurya
Copy link
Contributor Author

Do you think the benefit of this change is worth taking on the technical debt of caring this change forever?

Depends on the cost of the "technical debt". Currently I have no clue what that cost will be; it may be none (e.g. it just works, and there's no merge conflicts in the future), or it may have a cost to it (e.g. it doesn't work properly, and/or it results in merge conflicts when merging with upstream).

With that said, I believe this same behaviour can be achieve via some configuration files, similar to how we configure other cloud-init configuration. So, instead of modifying this in the code here, we could modify our DE image configuration to do the same thing. This assumes we actually want to regenerate the default cloud-init network configuration on each boot, which as I said, I don't think is required.

Since there's some doubt w.r.t. this change, and since I don't think it'll ultimately be required, I might as well close this and not land it. I don't see good reason to land an optional change, if there's legitimate concerns about its integration.

If we later determine we have a good reason to always regenerate the network config on all boots, we can reconsider this, but until then I'd probably recommend we drop this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants