Skip to content

Commit

Permalink
Merge pull request juju#17648 from barrettj12/merge-3.4-3.5-20240704
Browse files Browse the repository at this point in the history
juju#17648

Merges the following patches:
- juju#17645
- juju#17618
- juju#17568

### Conflicts
None.
  • Loading branch information
jujubot authored Jul 4, 2024
2 parents 0df4151 + 1acc0f5 commit b51f6b5
Show file tree
Hide file tree
Showing 17 changed files with 50 additions and 39 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: "Set up Go"
uses: actions/setup-go@v5
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/client-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: "Set up Go"
uses: actions/setup-go@v5
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:

steps:
- name: Checkout code
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Set up Go
uses: actions/setup-go@v5
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/gen.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: "Set up Go"
uses: actions/setup-go@v5
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/license.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jobs:
if: github.event.pull_request.draft == false
steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4
- name: Set up Go
uses: actions/setup-go@v5
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/merge.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
echo "target=$TARGET_BRANCH" >> "$GITHUB_OUTPUT"
- name: Checkout repository
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
fetch-depth: 0
ref: ${{ steps.branch.outputs.source }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/microk8s-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:

steps:
- name: Checking out repo
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Set up Go
uses: actions/setup-go@v5
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/migrate.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:

steps:
- name: Checkout code
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Set up Go
uses: actions/setup-go@v5
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/smoke.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
sudo DEBIAN_FRONTEND=noninteractive apt install -y expect
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Setup LXD
if: matrix.cloud == 'localhost'
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/snap.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
echo "/snap/bin" >> $GITHUB_PATH
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Setup LXD
uses: canonical/setup-lxd@4e959f8e0d9c5feb27d44c5e4d9a330a782edee0
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/static-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jobs:
if: github.event.pull_request.draft == false
steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Determine which tests to run
uses: dorny/paths-filter@v3
Expand Down Expand Up @@ -101,7 +101,7 @@ jobs:
if: github.event.pull_request.draft == false
steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Check if there is anything to test
uses: dorny/paths-filter@v3
Expand Down Expand Up @@ -160,10 +160,10 @@ jobs:
binary: ["jujud", "juju"]
steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Set up Go
uses: actions/setup-go@v4
uses: actions/setup-go@v5
with:
go-version-file: 'go.mod'
cache: true
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/terraform-smoke.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
sudo DEBIAN_FRONTEND=noninteractive apt install -y expect
- name: Checkout juju
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Setup LXD
uses: canonical/setup-lxd@4e959f8e0d9c5feb27d44c5e4d9a330a782edee0
Expand Down Expand Up @@ -61,7 +61,7 @@ jobs:
juju version
- name: Find terraform provider for juju latest release
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
repository: 'juju/terraform-provider-juju'
#path: terraform-provider-juju
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/upgrade.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
channel: ${{ steps.check.outputs.channel }}
steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4
- name: Check
id: check
run: |
Expand Down Expand Up @@ -62,7 +62,7 @@ jobs:

steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Setup LXD
if: matrix.cloud == 'localhost'
Expand Down
7 changes: 7 additions & 0 deletions PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
<!--
The PR title should match: <type>(optional <scope>): <description>.
Please also ensure all commits in this PR comply with our conventional commits specification:
https://docs.google.com/document/d/1SYUo9G7qZ_jdoVXpUVamS5VCgHmtZ0QA-wZxKoMS-C0
-->

<!-- Why this change is needed and what it does. -->

## Checklist
Expand Down
11 changes: 11 additions & 0 deletions provider/openstack/firewaller.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@ type Firewaller interface {
// If the model security group doesn't exist, return a NotFound error
ModelIngressRules(ctx context.ProviderCallContext) (firewall.IngressRules, error)

// DeleteMachineGroup delete's the security group specific to the provided machine.
// When in 'instance' firewall mode, each instance in a model is assigned its own
// security group, with a lifecycle matching that of the instance itself.
// In 'global' mode, all security groups are model scoped, and have lifecycles
// matching the model, so this method will remove no groups.
DeleteMachineGroup(ctx context.ProviderCallContext, machineId string) error

// DeleteAllModelGroups deletes all security groups for the
// model.
DeleteAllModelGroups(ctx context.ProviderCallContext) error
Expand Down Expand Up @@ -463,6 +470,10 @@ func (c *neutronFirewaller) DeleteAllModelGroups(ctx context.ProviderCallContext
return deleteSecurityGroupsMatchingName(ctx, c.deleteSecurityGroups, c.jujuGroupPrefixRegexp())
}

func (c *neutronFirewaller) DeleteMachineGroup(ctx context.ProviderCallContext, machineID string) error {
return deleteSecurityGroupsMatchingName(ctx, c.deleteSecurityGroups, c.machineGroupRegexp(machineID))
}

// UpdateGroupController implements Firewaller interface.
func (c *neutronFirewaller) UpdateGroupController(ctx context.ProviderCallContext, controllerUUID string) error {
neutronClient := c.environ.neutron()
Expand Down
20 changes: 13 additions & 7 deletions provider/openstack/local_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,7 @@ func (s *localServerSuite) TestStartInstanceWaitForActiveDetails(c *gc.C) {
c.Assert(insts, gc.HasLen, 0, gc.Commentf("expected launched instance to be terminated if stuck in BUILD state"))
}

func (s *localServerSuite) TestStartInstanceDeletesSecurityGroupsOnInstanceCreateFailure(c *gc.C) {
func (s *localServerSuite) TestStartInstanceDeletesMachineSecurityGroupOnInstanceCreateFailure(c *gc.C) {
env := s.openEnviron(c, coretesting.Attrs{"firewall-mode": config.FwInstance})

// Force an error in waitForActiveServerDetails
Expand All @@ -864,7 +864,10 @@ func (s *localServerSuite) TestStartInstanceDeletesSecurityGroupsOnInstanceCreat
c.Check(inst, gc.IsNil)
c.Assert(err, gc.NotNil)

assertSecurityGroups(c, env, []string{"default"})
assertSecurityGroups(c, env, []string{
fmt.Sprintf("juju-%s-%s", coretesting.ControllerTag.Id(), coretesting.ModelTag.Id()),
"default",
})
}

func (s *localServerSuite) TestStartInstanceDeletesSecurityGroupsOnFailure(c *gc.C) {
Expand All @@ -881,7 +884,10 @@ func (s *localServerSuite) TestStartInstanceDeletesSecurityGroupsOnFailure(c *gc
_, _, _, err := testing.StartInstance(env, s.callCtx, s.ControllerUUID, "100")
c.Assert(err, gc.NotNil)

assertSecurityGroups(c, env, []string{"default"})
assertSecurityGroups(c, env, []string{
fmt.Sprintf("juju-%s-%s", coretesting.ControllerTag.Id(), coretesting.ModelTag.Id()),
"default",
})
}

func assertSecurityGroups(c *gc.C, env environs.Environ, expected []string) {
Expand Down Expand Up @@ -3062,8 +3068,8 @@ func (s *localServerSuite) TestUpdateGroupController(c *gc.C) {
c.Assert(err, jc.ErrorIsNil)
groupNamesBefore := set.NewStrings(groupNames...)
c.Assert(groupNamesBefore, gc.DeepEquals, set.NewStrings(
"juju-deadbeef-1bad-500d-9000-4b1d0d06f00d-deadbeef-0bad-400d-8000-4b1d0d06f00d",
"juju-deadbeef-1bad-500d-9000-4b1d0d06f00d-deadbeef-0bad-400d-8000-4b1d0d06f00d-0",
fmt.Sprintf("juju-%s-%s", coretesting.ControllerTag.Id(), coretesting.ModelTag.Id()),
fmt.Sprintf("juju-%s-%s-0", coretesting.ControllerTag.Id(), coretesting.ModelTag.Id()),
))

firewaller := openstack.GetFirewaller(s.env)
Expand All @@ -3074,8 +3080,8 @@ func (s *localServerSuite) TestUpdateGroupController(c *gc.C) {
c.Assert(err, jc.ErrorIsNil)
groupNamesAfter := set.NewStrings(groupNames...)
c.Assert(groupNamesAfter, gc.DeepEquals, set.NewStrings(
"juju-aabbccdd-eeee-ffff-0000-0123456789ab-deadbeef-0bad-400d-8000-4b1d0d06f00d",
"juju-aabbccdd-eeee-ffff-0000-0123456789ab-deadbeef-0bad-400d-8000-4b1d0d06f00d-0",
fmt.Sprintf("juju-aabbccdd-eeee-ffff-0000-0123456789ab-%s", coretesting.ModelTag.Id()),
fmt.Sprintf("juju-aabbccdd-eeee-ffff-0000-0123456789ab-%s-0", coretesting.ModelTag.Id()),
))
}

Expand Down
15 changes: 1 addition & 14 deletions provider/openstack/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -1273,7 +1273,7 @@ func (e *Environ) startInstance(
server, err := tryStartNovaInstance(shortAttempt, e.nova(), opts)
if err != nil || server == nil {
// Attempt to clean up any security groups we created.
if err := e.cleanupGroups(ctx, e.nova(), novaGroupNames); err != nil {
if err := e.firewaller.DeleteMachineGroup(ctx, args.InstanceConfig.MachineId); err != nil {
// If we failed to clean up the security groups, we need the user
// to manually clean them up.
logger.Errorf("cannot cleanup security groups: %v", err)
Expand Down Expand Up @@ -1351,19 +1351,6 @@ func (e *Environ) startInstance(
}, nil
}

// Clean up any groups that we have created if we fail to start the instance.
func (e *Environ) cleanupGroups(
ctx context.ProviderCallContext,
client *nova.Client,
groups []nova.SecurityGroupName,
) error {
names := make([]string, len(groups))
for i, group := range groups {
names[i] = group.Name
}
return e.firewaller.DeleteGroups(ctx, names...)
}

func (e *Environ) userFriendlyInvalidNetworkError(err error) error {
msg := fmt.Sprintf("%s\n\t%s\n\t%s", err.Error(),
"This error was caused by juju attempting to create an OpenStack instance with no network defined.",
Expand Down

0 comments on commit b51f6b5

Please sign in to comment.