Merge ~rjschwei/cloud-init:noManualNet into cloud-init:master
| Status: | Needs review |
|---|---|
| Proposed branch: | ~rjschwei/cloud-init:noManualNet |
| Merge into: | cloud-init:master |
| Diff against target: |
13 lines (+1/-1) 1 file modified
cloudinit/distros/opensuse.py (+1/-1) |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Ryan Harper | 2017-11-24 | Needs Fixing on 2017-12-18 | |
| Server Team CI bot | continuous-integration | Approve on 2017-11-24 | |
|
Review via email:
|
|||
Description of the Change
Related to 1733169
A network interface in a cloud environment should not be set to manual startmode. It is possible to receive data from the data source where a network interface may not be marked as "auto". This used to trigger the network configuration to set the startmode to manual. Manual startmode in a cloud environment may trigger loss of access to the VM if the only interface is not brought up at boot time. This fix addresses the problem by setting the startmode to "hotplug". Thus if the interface exists it will be started, even if it is not marked as "auto" in the data that describes the interface.
| Scott Moser (smoser) wrote : | # |
Robert,
This looks fine to me.
We clearly do not have very good unit tests on the suse path.
Otherwise you'd have broken a test with your change.
I do approve of the change, but can we get a test added?
If you want to ping me in cloud-init today i can point you in the right direction.
| Robert Schweikert (rjschwei) wrote : | # |
> Robert,
> This looks fine to me.
> We clearly do not have very good unit tests on the suse path.
Agreed
> Otherwise you'd have broken a test with your change.
>
> I do approve of the change, but can we get a test added?
Well the rub here is that I would really like to get the SUSE network path implementation onto the "renderer concept" code path. But I have not had as much time as I'd like to for working out a proposal about what to do with syconfig renderer to make it work with both SLES and RHEL.
| Robert Schweikert (rjschwei) wrote : | # |
Oh, and I forgot, related to this I did submit a test
https:/
not as rich as it probably should be but it's a start, and this has been sitting for a while :(
| Ryan Harper (raharper) wrote : | # |
The submitted test doesn't cover 'manual' vs 'hotplug'. Either bring in a test for this specific change in this branch, or extend the test-case in the other to include the 'hotplug' vs 'manual' change.
| Robert Schweikert (rjschwei) wrote : | # |
OK, so maybe I am missing something here, beside the fact that in a cloud environment setting an interface to "manual" for bring-up mode is rather silly.
When I hit this issue, if I recall correctly, we ended up coming to the conclusion that the root cause was that here is no path from the v1 network description to the "translate" code path. This I tried to address in https:/
Since this request is not going anywhere either I can hardly add a test to the other MP to test that things get set to "hotplug" as I would then have a failing test which is a no-no for acceptance. Thus now I am in a thread-lock condition.
I understand that the SUSE code path lacks tests and that there are other issues floating around with respect to code cleanliness and structure with respect to the way we want them to be. However, I cannot boil the ocean, and I cannot add all the tests we would like to have on the SUSE code path at once, there is simply not enough time in my day. Ending up in a thread lock situation doesn't help me move forward either. There has got to be some way where things can get accepted and merged with a "needs explicit test" and I don't care if there is a bug filed and assigned to me. But this is just.......
| Ryan Harper (raharper) wrote : | # |
On Mon, Dec 18, 2017 at 1:21 PM, Robert Schweikert <email address hidden>
wrote:
> OK, so maybe I am missing something here, beside the fact that in a cloud
> environment setting an interface to "manual" for bring-up mode is rather
> silly.
>
> When I hit this issue, if I recall correctly, we ended up coming to the
> conclusion that the root cause was that here is no path from the v1 network
> description to the "translate" code path. This I tried to address in
> https:/
> cloud-init/
> because of me missing Chad's response on 11/29.
>
> Since this request is not going anywhere either I can hardly add a test to
> the other MP to test that things get set to "hotplug" as I would then have
> a failing test which is a no-no for acceptance. Thus now I am in a
> thread-lock condition.
>
It's certainly fine to order the commits if you don't merge them. The
other branch can certainly validate what the ifcfg-ethX file should look
like, and without this patch, that would include the 'manual' value. And
in this branch
which we can specify to be merged after the other would update the test to
flip the value of from 'manual' to 'hotplug' as needed.
If we want to avoid the deadload, then a different unit test which examines
the current output without the translate path, it still writes some
configuration files with the 'manual' value,
here look at tests/unittests
at the bottom is the _freebsd one, which is basically what you need to do
for opensuse.
replace the get_distro value to opensuse and then feed it the busted config
from the bug a
and show that it renders MODE=hotplug.
| Robert Schweikert (rjschwei) wrote : | # |
Sorry, I just don't want to do work that I then have to redo again or change in yet another MP that spins in another loop.
Unmerged commits
- c62bda0... by Robert Schweikert on 2017-11-24
PASSED: Continuous integration, rev:c62bda04858
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/