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

Remove ContainerBridgeName bootstrap configuration #13414

Merged
merged 8 commits into from
Oct 14, 2021

Conversation

manadart
Copy link
Member

@manadart manadart commented Oct 13, 2021

Once upon a time we pro-actively set up a juju-br0, and allowed the user to change the default name via the bootstrap configuration article ContainerBridgeName.

It is long since Juju worked in this way, but we have retained a bunch of unnecessary logic around it.

Here we do the following:

  • Remove the ControllerBridgeName configuration item.
  • Remove the logic to pass it through to agent configuration.
  • Remove the downstream fall-backs to this device name.

This is all safe, because the bridge policy logic throws errors on all paths where we fail to determine the correct device to use, or to generate a NIC configuration to pass into container creation logic - we never use the fall-back logic path.

QA steps

  • Bootstrap to LXD and add some workloads.
  • Bootstrap to AWS (Fan container networking).
    • Create a space.
    • Deploy LXD-in-machine into this space using a constraint.
  • Bootstrap to MAAS (provider container networking).
    • Deploy LXD-in-machine into this space using a constraint.
    • Deploy KVM-in-machine into this space using a constraint.
    • Deploy LXD-in-machine using an unsatisfyable space constraint and ensure the correct provisioning error.

Documentation changes

None.

Bug reference

N/A

retrieve agent config for bridges, because it is never set.
This should never be encountered, because way upstream we ensure that
the input interfaces are never empty.
This is always set during finishNetworkConfig, which precedes the call
to this method in all cases.
It is not used anywhere downstream and is therefore removed from
container.NetworkConfig.
@manadart manadart added the 2.9 label Oct 13, 2021
@manadart
Copy link
Member Author

@manadart manadart force-pushed the 2.9-remove-container-bridge-name branch from c224d8e to 964b085 Compare October 13, 2021 13:11
Copy link
Member

@hmlanigan hmlanigan left a comment

Choose a reason for hiding this comment

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

One question on the changes - still QAing

container/broker/lxd-broker.go Outdated Show resolved Hide resolved
@manadart manadart force-pushed the 2.9-remove-container-bridge-name branch from 964b085 to 1188dde Compare October 13, 2021 14:24
We should never be in the fallback case based on bridge policy logic.
@hmlanigan
Copy link
Member

Tested on lxd, aws and openstack, but not Maas

@manadart
Copy link
Member Author

$$merge$$

@jujubot jujubot merged commit 3bb83ca into juju:2.9 Oct 14, 2021
@manadart manadart deleted the 2.9-remove-container-bridge-name branch October 14, 2021 14:13
@wallyworld wallyworld mentioned this pull request Oct 15, 2021
jujubot added a commit that referenced this pull request Oct 15, 2021
#13421

Merge 2.9

#13406 Take into account manual machine arches when validating constraints
#13409 Fix podspec deployment/statefulset/daemonset upgrade.
#13410 update-k8s also updates any controller proxy
#13411 Acceptance tests - move away from unsupported series
#13402 grab bag of metrics updates
#13414 Remove ContainerBridgeName bootstrap configuration
#13415 Introduce assumes expression sat checker
#13420 Sync tools help msg
#13413 Worker v3 fixes

The restorewatcher files in conflict have been deleted from develop branch.

```
# Conflicts:
# apiserver/params/charms.go
# caas/kubernetes/provider/bootstrap_test.go
# charmhub/refresh.go
# go.mod
# go.sum
# state/open.go
# worker/restorewatcher/manifold.go
# worker/restorewatcher/manifold_test.go
# worker/restorewatcher/worker.go
# worker/restorewatcher/worker_test.go
```

## QA steps

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

Successfully merging this pull request may close these issues.

3 participants