Skip to content

Make wakeonlan Network Config v2 setting actually work#626

Merged
OddBloke merged 1 commit into
canonical:masterfrom
dermotbradley:fix-wakeonlan
Nov 9, 2020
Merged

Make wakeonlan Network Config v2 setting actually work#626
OddBloke merged 1 commit into
canonical:masterfrom
dermotbradley:fix-wakeonlan

Conversation

@dermotbradley
Copy link
Copy Markdown
Contributor

Proposed Commit Message

Add code so that specifying "wakeonlan: true" actually results in relevant
configuration entry appearing in /etc/network/interfaces, NetPlan, and
sysconfig for RHEL and OpenSuse.

Add testcases for the above.

Additional Context

Test Steps

In a Network Config v2 YAML file specify the following:

version: 2
ethernets:
    eth0:
        dhcp4: true
        wakeonlan: true

Then check the resultant Netplan file to see if wakeonlan specified there.

Wakeonlan is not supported in a v1 config file.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@igalic
Copy link
Copy Markdown
Collaborator

igalic commented Oct 24, 2020

if you're feeling very generous, you could try to add this to the bsd renderer as well!

@dermotbradley
Copy link
Copy Markdown
Contributor Author

if you're feeling very generous, you could try to add this to the bsd renderer as well!

Hi Mina. I'm unfamiliar with how any of the BSDs manage WoL so I didn't want to try that. Also tests/unittests/test_net.py doesn't appear to have any existing testcases for the BSDs and I didn't want to be the first person to try and add them to it.

Whilst I don't currently use RHEL, Opensuse/SLES, or Ubuntu (Netplan) I had enough previously familiarity with them to add their support in addition to the /etc/network/interface support which I currently use on both Alpine and Debian.

@igalic
Copy link
Copy Markdown
Collaborator

igalic commented Oct 25, 2020

i could provide a patch against your branch for wake-on-lan, but, yeah, tests, especially good ones, are currently a rarity

@dermotbradley
Copy link
Copy Markdown
Contributor Author

i could provide a patch against your branch for wake-on-lan, but, yeah, tests, especially good ones, are currently a rarity

I'd prefer to get this PR merged and then you could raise a further PR for BSD-specific support.

Comment thread tests/unittests/test_net.py Outdated
dhcp4: true
wakeonlan: true
version: 2
""").rstrip(' '),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

dedent is weird sometimes.

I don't htink you should need the rstrip here, and think you might have added it in an attempt to combat misuse of dedent.

You really need the \ on line 1359. dedent will remove the same number of spaces from the beginning of each line. but in this stanza, you your first line has no indentation (it is just "\n"). Add the \ and I think you can drop the rstrip.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd copied (and modified) this testcase structure from "dhcpv6_stateless" and so that's inherited from it.

I'll test your suggestions and update accordingly.

Copy link
Copy Markdown
Contributor Author

@dermotbradley dermotbradley Oct 28, 2020

Choose a reason for hiding this comment

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

I've removed the rstrip.

However if I add the trailing \ to line 1359 the testcase fails - seems there needs to be a blank line before the "network:". There are other testcases in this file that are the same (I copied from one of them).

Copy link
Copy Markdown
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

i don't have much to contribute here today…

Comment thread cloudinit/net/network_state.py Outdated
Comment thread cloudinit/net/sysconfig.py Outdated
Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

Thanks for the submission, Dermot! I really appreciate you taking the time to fix this across the multiple networking backends we support. 👍

I've identified a common issue, I think: if wakeonlan is specified as false, then the code still enables WOL (except for netplan, where it's just passed through). I may have got the wrong end of the stick on that, but either way, I think I'd like to see testing that setting it to false results in a standard network config being rendered.

(Aside from that, I have a comment on the sysconfig implementation specifically, inline.)

Thanks again!

Comment thread tests/unittests/test_net.py
Comment thread tests/unittests/test_net.py
Comment thread tests/unittests/test_net.py
Comment thread tests/unittests/test_net.py
Comment thread cloudinit/net/eni.py Outdated
Comment thread cloudinit/net/sysconfig.py Outdated
@igalic
Copy link
Copy Markdown
Collaborator

igalic commented Nov 7, 2020

@dermotbradley you should also do a rebase.

Add code so that specifying "wakeonlan: true" actually results in relevant
configuration entry appearing in /etc/network/interfaces, NetPlan, and
sysconfig for RHEL and OpenSuse.

Add testcases for the above.
@dermotbradley
Copy link
Copy Markdown
Contributor Author

All taken care of and rebased from master.

Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@OddBloke OddBloke merged commit 57349eb into canonical:master Nov 9, 2020
@dermotbradley dermotbradley deleted the fix-wakeonlan branch November 18, 2020 18:20
TheRealFalcon added a commit to TheRealFalcon/cloud-init that referenced this pull request Dec 14, 2020
Ensure if wakeonlan is specified in the network config that it is
rendered in the /etc/network/interfaces or netplan config.
@TheRealFalcon TheRealFalcon mentioned this pull request Dec 14, 2020
3 tasks
TheRealFalcon added a commit to TheRealFalcon/cloud-init that referenced this pull request Dec 14, 2020
Ensure if wakeonlan is specified in the network config that it is
rendered in the /etc/network/interfaces or netplan config.
OddBloke pushed a commit that referenced this pull request Dec 15, 2020
Ensure if wakeonlan is specified in the network config that it is
rendered in the /etc/network/interfaces or netplan config.
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.

4 participants