From 1325a0d95d296c0727a128196f5f30f1cc91172c Mon Sep 17 00:00:00 2001 From: Chris Privitere Date: Mon, 30 Jan 2023 17:07:44 -0600 Subject: [PATCH 01/10] Allow 10.0.0.0/8 traffic Add iptables rule to enable 10.0.0.0/8 traffic. This allows other equinix project members to talk to each other. Signed-off-by: Chris Privitere --- provider/equinix/environ.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/provider/equinix/environ.go b/provider/equinix/environ.go index 1dd1f66f8b4..4689b2fb390 100644 --- a/provider/equinix/environ.go +++ b/provider/equinix/environ.go @@ -274,7 +274,8 @@ func getCloudConfig(args environs.StartInstanceParams) (cloudinit.CloudConfig, e } cloudCfg.AddPackage("iptables-persistent") - // Set a default INPUT policy of drop, permitting ssh + // Set a default INPUT policy of drop, permitting ssh and 10.0.0.0/8 private + // network traffic. acceptInputPort := "iptables -A INPUT -p tcp --dport %d -j ACCEPT" iptablesDefault := []string{ "iptables -A INPUT -m conntrack --ctstate INVALID -j DROP", @@ -298,6 +299,7 @@ func getCloudConfig(args environs.StartInstanceParams) (cloudinit.CloudConfig, e } } } + iptablesDefault = append(iptablesDefault, "iptables -A INPUT -s 10.0.0.0/8 -j ACCEPT") iptablesDefault = append(iptablesDefault, "iptables -A INPUT -j DROP") cloudCfg.AddScripts( From 222130e8afb270a84674d94a228398c0a05bf9e3 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Thu, 2 Feb 2023 15:47:36 -0500 Subject: [PATCH 02/10] Allow a application's channel to be updated even if the charm url is not. Fix for LP1988587. With charm store you could update the channel to be used in the future, even if the charm itself didn't require upgrade. Allow the same behavior for charm hub charms. --- cmd/juju/application/refresh.go | 18 +++++++++--------- cmd/juju/application/refresh_test.go | 25 +++++++++++++++++++++++++ state/application.go | 1 + state/application_test.go | 20 ++++++++++++++++++++ 4 files changed, 55 insertions(+), 9 deletions(-) diff --git a/cmd/juju/application/refresh.go b/cmd/juju/application/refresh.go index 4af75c5a032..3fbc66b80df 100644 --- a/cmd/juju/application/refresh.go +++ b/cmd/juju/application/refresh.go @@ -430,7 +430,8 @@ func (c *refreshCommand) Run(ctx *cmd.Context) error { return errors.Trace(err) } charmID, err := factory.Run(cfg) - if err == nil { + switch { + case err == nil: curl := charmID.URL charmOrigin := charmID.Origin // The current charm URL that's been found and selected. @@ -439,13 +440,13 @@ func (c *refreshCommand) Run(ctx *cmd.Context) error { channel = fmt.Sprintf(" in channel %s", charmID.Origin.Channel.String()) } ctx.Infof("Added %s charm %q, revision %d%s, to the model", charmOrigin.Source, curl.Name, curl.Revision, channel) - } else if errors.Is(err, refresher.ErrAlreadyUpToDate) { - if len(c.Resources) == 0 { - // Charm already up-to-date and no resources to refresh. - ctx.Infof(err.Error()) - return nil - } - } else { + case errors.Is(err, refresher.ErrAlreadyUpToDate) && c.Channel.String() != oldOrigin.CoreCharmOrigin().Channel.String(): + ctx.Infof("%s. Note: all future refreshes will now use channel %q", err.Error(), charmID.Origin.Channel.String()) + case errors.Is(err, refresher.ErrAlreadyUpToDate) && len(c.Resources) == 0: + // Charm already up-to-date and no resources to refresh. + ctx.Infof(err.Error()) + return nil + default: if termErr, ok := errors.Cause(err).(*common.TermsRequiredError); ok { return errors.Trace(termErr.UserErr()) } @@ -453,7 +454,6 @@ func (c *refreshCommand) Run(ctx *cmd.Context) error { } // Next, upgrade resources. - resourceLister, err := c.NewResourceLister(apiRoot) if err != nil { return errors.Trace(err) diff --git a/cmd/juju/application/refresh_test.go b/cmd/juju/application/refresh_test.go index 293981b8503..369c2349c10 100644 --- a/cmd/juju/application/refresh_test.go +++ b/cmd/juju/application/refresh_test.go @@ -149,6 +149,9 @@ func (s *BaseRefreshSuite) SetUpTest(c *gc.C) { bindings: map[string]string{ "": network.AlphaSpaceName, }, + charmOrigin: commoncharm.Origin{ + Risk: "stable", + }, } s.modelConfigGetter = newMockModelConfigGetter() s.resourceLister = mockResourceLister{} @@ -620,6 +623,28 @@ func (s *RefreshSuite) TestUpgradeWithChannel(c *gc.C) { }) } +func (s *RefreshSuite) TestUpgradeWithChannelNoNewCharmURL(c *gc.C) { + // Test setting a new charm channel, without an actual + // charm upgrade needed. + s.resolvedCharmURL = charm.MustParseURL("cs:quantal/foo-1") + s.resolvedChannel = csclientparams.BetaChannel + _, err := s.runRefresh(c, "foo", "--channel=beta") + c.Assert(err, jc.ErrorIsNil) + + s.charmAPIClient.CheckCallNames(c, "GetCharmURLOrigin", "Get", "SetCharm") + s.charmAPIClient.CheckCall(c, 2, "SetCharm", model.GenerationMaster, application.SetCharmConfig{ + ApplicationName: "foo", + CharmID: application.CharmID{ + URL: s.resolvedCharmURL, + Origin: commoncharm.Origin{ + Source: "charm-store", + Architecture: arch.DefaultArchitecture, + Risk: "beta", + }, + }, + }) +} + func (s *RefreshSuite) TestRefreshShouldRespectDeployedChannelByDefault(c *gc.C) { s.resolvedChannel = csclientparams.BetaChannel _, err := s.runRefresh(c, "foo") diff --git a/state/application.go b/state/application.go index 5170e47d82a..65fcb603526 100644 --- a/state/application.go +++ b/state/application.go @@ -1643,6 +1643,7 @@ func (a *Application) SetCharm(cfg SetCharmConfig) (err error) { Assert: txn.DocExists, Update: bson.D{{"$set", bson.D{ {"cs-channel", channel}, + {"charm-origin.channel", cfg.CharmOrigin.Channel}, {"forcecharm", cfg.ForceUnits}, }}}, }) diff --git a/state/application_test.go b/state/application_test.go index f173b17092b..1da8a37bb63 100644 --- a/state/application_test.go +++ b/state/application_test.go @@ -141,6 +141,26 @@ func (s *ApplicationSuite) TestSetCharmSeries(c *gc.C) { c.Assert(s.mysql.Series(), gc.DeepEquals, "new-series") } +func (s *ApplicationSuite) TestSetCharmUpdateChannelURLNoChange(c *gc.C) { + sch := s.AddMetaCharm(c, "mysql", metaBase, 2) + + origin := s.mysql.CharmOrigin() + origin.Channel = &state.Channel{Risk: "stable"} + + cfg := state.SetCharmConfig{ + Charm: sch, + CharmOrigin: origin, + } + err := s.mysql.SetCharm(cfg) + c.Assert(err, jc.ErrorIsNil) + c.Assert(s.mysql.CharmOrigin().Channel.Risk, gc.DeepEquals, "stable") + + cfg.CharmOrigin.Channel.Risk = "candidate" + err = s.mysql.SetCharm(cfg) + c.Assert(err, jc.ErrorIsNil) + c.Assert(s.mysql.CharmOrigin().Channel.Risk, gc.DeepEquals, "candidate") +} + func (s *ApplicationSuite) TestSetCharmCharmOriginNoChange(c *gc.C) { // Add a compatible charm. sch := s.AddMetaCharm(c, "mysql", metaBase, 2) From 9c50fac500364a65341351d5cfe5c7fbcdd4534a Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Fri, 3 Feb 2023 15:27:36 -0500 Subject: [PATCH 03/10] Saved by unit tests, ensure we do not panic on nil pointer. Unit tests and SetCharm called by the Update application facade method will not contain a charm origin. Ensure we don't panic on this in the real world. The Update facade method is only kept for compatibility with older juju 2.x clients. With juju 3.0, we can revaluate checking for an empty charm origin in the facade and updating the unit tests. --- state/application.go | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/state/application.go b/state/application.go index 65fcb603526..36df8fe0db0 100644 --- a/state/application.go +++ b/state/application.go @@ -1636,16 +1636,24 @@ func (a *Application) SetCharm(cfg SetCharmConfig) (err error) { }} if *a.doc.CharmURL == cfg.Charm.URL().String() { + updates := bson.D{ + {"cs-channel", channel}, + {"forcecharm", cfg.ForceUnits}, + } + // Local charms will not have a channel in their charm origin + // TODO: (hml) 2023-02-03 + // With juju 3.0, SetCharm should always have a CharmOrigin. + // Compatibility with the Update application facade method + // is no longer necessary. + if cfg.CharmOrigin != nil && cfg.CharmOrigin.Channel != nil { + updates = append(updates, bson.DocElem{"charm-origin.channel", cfg.CharmOrigin.Channel}) + } // Charm URL already set; just update the force flag and channel. ops = append(ops, txn.Op{ C: applicationsC, Id: a.doc.DocID, Assert: txn.DocExists, - Update: bson.D{{"$set", bson.D{ - {"cs-channel", channel}, - {"charm-origin.channel", cfg.CharmOrigin.Channel}, - {"forcecharm", cfg.ForceUnits}, - }}}, + Update: bson.D{{"$set", updates}}, }) } else { // Check if the new charm specifies a relation max limit @@ -1676,6 +1684,10 @@ func (a *Application) SetCharm(cfg SetCharmConfig) (err error) { newCharmModifiedVersion++ } + // TODO: (hml) 2023-02-03 + // With juju 3.0, SetCharm should always have a CharmOrigin. + // Compatibility with the Update application facade method + // is no longer necessary. Modify checks appropriately. if cfg.CharmOrigin != nil { // Update in the application facade also calls // SetCharm, though it has no current user in the @@ -1720,7 +1732,6 @@ func (a *Application) SetCharm(cfg SetCharmConfig) (err error) { // ErrNoOperations on the other hand means there's nothing to update. return nil, errors.Trace(err) } - return ops, nil } From 27a4253cdc148e0ae3f5b8ac36cc65bfb6b6ebd4 Mon Sep 17 00:00:00 2001 From: Joseph Phillips Date: Mon, 6 Feb 2023 12:30:23 +0100 Subject: [PATCH 04/10] Ensures that the logic in State.removeAllModelDocs only recruits low-level Mongo collection access and not business-logic-level constructs. These constructs assume data integrity, which is unlikely to be present when attempting to remove data for a failed migration. The change here ensures that we successfully roll back a migration that fails while importing relations. --- state/applicationoffers.go | 2 +- state/state.go | 76 +++++++++++++++++++++----------------- 2 files changed, 43 insertions(+), 35 deletions(-) diff --git a/state/applicationoffers.go b/state/applicationoffers.go index f438c89e8c4..6310e07e7c3 100644 --- a/state/applicationoffers.go +++ b/state/applicationoffers.go @@ -143,7 +143,7 @@ func (s *applicationOffers) AllApplicationOffers() (offers []*crossmodel.Applica var docs []applicationOfferDoc err := applicationOffersCollection.Find(bson.D{}).All(&docs) if err != nil { - return nil, errors.Errorf("cannot get all application offers") + return nil, errors.Annotate(err, "getting application offer documents") } for _, doc := range docs { offer, err := s.makeApplicationOffer(doc) diff --git a/state/state.go b/state/state.go index ba5809df96b..44e31611239 100644 --- a/state/state.go +++ b/state/state.go @@ -240,41 +240,14 @@ func (st *State) RemoveExportingModelDocs() error { } func (st *State) removeAllModelDocs(modelAssertion bson.D) error { - modelUUID := st.ModelUUID() - - // Gather all user permissions for the model. - // Do this first because we remove some parent docs below. - var permOps []txn.Op - permPattern := bson.M{ - "_id": bson.M{"$regex": "^" + permissionID(modelKey(modelUUID), "")}, - } - ops, err := st.removeInCollectionOps(permissionsC, permPattern) - if err != nil { - return errors.Trace(err) - } - permOps = append(permOps, ops...) - // Gather all offer permissions for the model. - ao := NewApplicationOffers(st) - allOffers, err := ao.AllApplicationOffers() - if err != nil { - return errors.Trace(err) - } - for _, offer := range allOffers { - permPattern = bson.M{ - "_id": bson.M{"$regex": "^" + permissionID(applicationOfferKey(offer.OfferUUID), "")}, - } - ops, err = st.removeInCollectionOps(permissionsC, permPattern) - if err != nil { - return errors.Trace(err) - } - permOps = append(permOps, ops...) - } - err = st.db().RunTransaction(permOps) - if err != nil { - return errors.Trace(err) + // Remove permissions first, because we potentially + // remove parent documents in the following stage. + if err := st.removeAllModelPermissions(); err != nil { + return errors.Annotate(err, "removing permissions") } // Remove each collection in its own transaction. + modelUUID := st.ModelUUID() for name, info := range st.database.Schema() { if info.global || info.rawAccess { continue @@ -317,7 +290,7 @@ func (st *State) removeAllModelDocs(modelAssertion bson.D) error { if err != nil { return errors.Trace(err) } - ops = []txn.Op{{ + ops := []txn.Op{{ // Cleanup the owner:envName unique key. C: usermodelnameC, Id: model.uniqueIndexID(), @@ -343,7 +316,42 @@ func (st *State) removeAllModelDocs(modelAssertion bson.D) error { if !st.IsController() { ops = append(ops, decHostedModelCountOp()) } - return st.db().RunTransaction(ops) + return errors.Trace(st.db().RunTransaction(ops)) +} + +// removeAllModelPermissions removes all direct permissions documents for +// this model, and all permissions for offers hosted by this model. +func (st *State) removeAllModelPermissions() error { + var permOps []txn.Op + permPattern := bson.M{ + "_id": bson.M{"$regex": "^" + permissionID(modelKey(st.ModelUUID()), "")}, + } + ops, err := st.removeInCollectionOps(permissionsC, permPattern) + if err != nil { + return errors.Trace(err) + } + permOps = append(permOps, ops...) + + applicationOffersCollection, closer := st.db().GetCollection(applicationOffersC) + defer closer() + + var offerDocs []applicationOfferDoc + if err := applicationOffersCollection.Find(bson.D{}).All(&offerDocs); err != nil { + return errors.Annotate(err, "getting application offer documents") + } + + for _, offer := range offerDocs { + permPattern = bson.M{ + "_id": bson.M{"$regex": "^" + permissionID(applicationOfferKey(offer.OfferUUID), "")}, + } + ops, err = st.removeInCollectionOps(permissionsC, permPattern) + if err != nil { + return errors.Trace(err) + } + permOps = append(permOps, ops...) + } + err = st.db().RunTransaction(permOps) + return errors.Trace(err) } // removeAllInCollectionRaw removes all the documents from the given From 0ab03431b7f93b8b5b10858ff97a3510d8e1ce0c Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Mon, 6 Feb 2023 15:01:32 -0500 Subject: [PATCH 05/10] Keep going if not revision change, but resources have been specified. A miss in commit to fix LP1988587. --- cmd/juju/application/refresh.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/juju/application/refresh.go b/cmd/juju/application/refresh.go index 3fbc66b80df..149d6a63d37 100644 --- a/cmd/juju/application/refresh.go +++ b/cmd/juju/application/refresh.go @@ -446,6 +446,8 @@ func (c *refreshCommand) Run(ctx *cmd.Context) error { // Charm already up-to-date and no resources to refresh. ctx.Infof(err.Error()) return nil + case errors.Is(err, refresher.ErrAlreadyUpToDate) && len(c.Resources) > 0: + ctx.Infof("%s. Attempt to update resources requested.", err.Error()) default: if termErr, ok := errors.Cause(err).(*common.TermsRequiredError); ok { return errors.Trace(termErr.UserErr()) From d2d03a331c7551ed3a4ce95ef1aba3a1b7c58728 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Mon, 6 Feb 2023 15:03:04 -0500 Subject: [PATCH 06/10] Add two new tests related to refresh without revision change. Test cover scenarios in LP1988587, where refresh failed due to a lack of new available charm revision, however the switch or channel flags were used. --- tests/suites/refresh/switch.sh | 74 ++++++++++++++++++++++++++++++++-- 1 file changed, 70 insertions(+), 4 deletions(-) diff --git a/tests/suites/refresh/switch.sh b/tests/suites/refresh/switch.sh index e6af164d2c1..6cf99526d30 100644 --- a/tests/suites/refresh/switch.sh +++ b/tests/suites/refresh/switch.sh @@ -19,7 +19,7 @@ run_refresh_switch_cs_to_ch() { # shellcheck disable=SC2059 printf "${OUT}\n" - # Added local charm "ubuntu", revision 2, to the model + # format: Added local charm "ubuntu", revision 2, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "ubuntu" "$(charm_rev "ubuntu" "${revision}")" @@ -28,6 +28,39 @@ run_refresh_switch_cs_to_ch() { destroy_model "${model_name}" } +run_refresh_switch_cs_to_ch_no_new_revision() { + # Test juju refresh from a charm store charm to a charm hub charm, with no new + # charm revision. + echo + + model_name="test-refresh-switch-ch" + file="${TEST_DIR}/${model_name}.log" + + ensure "${model_name}" "${file}" + + OUT=$(juju deploy cs:ubuntu2 >&1 || true) + # shellcheck disable=SC2059 + printf "${OUT}\n" + # format: Added local charm "ubuntu", revision 2, to the model + cs_revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') + + wait_for "ubuntu" "$(idle_condition "ubuntu")" + + OUT=$(juju refresh ubuntu --switch ch:ubuntu 2>&1 || true) + if echo "${OUT}" | grep -E -vq "Added charm-hub charm"; then + # shellcheck disable=SC2046 + echo $(red "failed refreshing charm: ${OUT}") + exit 5 + fi + # shellcheck disable=SC2059 + printf "${OUT}\n" + + wait_for "ubuntu" "$(charm_rev "ubuntu" "${cs_revision}")" + wait_for "ubuntu" "$(idle_condition "ubuntu")" + + destroy_model "${model_name}" +} + run_refresh_switch_cs_to_ch_channel() { # Test juju refresh from a charm store charm to a charm hub charm with a specific channel echo @@ -49,7 +82,7 @@ run_refresh_switch_cs_to_ch_channel() { # shellcheck disable=SC2059 printf "${OUT}\n" - # Added local charm "ubuntu", revision 2, to the model + # format: Added local charm "ubuntu", revision 2, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "ubuntu" "$(charm_rev "ubuntu" "${revision}")" @@ -82,7 +115,7 @@ run_refresh_switch_local_to_ch_channel() { # shellcheck disable=SC2059 printf "${OUT}\n" - # Added local charm "ubuntu", revision 2, to the model + # format: Added local charm "ubuntu", revision 2, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "ubuntu" "$(charm_rev "ubuntu" "${revision}")" @@ -108,7 +141,7 @@ run_refresh_switch_channel() { # shellcheck disable=SC2059 printf "${OUT}\n" - # Added local charm "ubuntu", revision 2, to the model + # format: Added local charm "ubuntu", revision 2, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "juju-qa-test" "$(charm_rev "juju-qa-test" "${revision}")" @@ -118,6 +151,37 @@ run_refresh_switch_channel() { destroy_model "${model_name}" } +run_refresh_switch_channel_no_new_revision() { + # Test juju refresh switching from one channel to another, with no new + # charm revision. + echo + + model_name="test-refresh-switch-channel-no-new-revision" + file="${TEST_DIR}/${model_name}.log" + + ensure "${model_name}" "${file}" + + juju deploy ubuntu + wait_for "ubuntu" "$(idle_condition "ubuntu")" + + OUT=$(juju refresh ubuntu --channel edge 2>&1 || true) + # shellcheck disable=SC2059 + printf "${OUT}\n" + + if echo "${OUT}" | grep -E -vq "all future refreshes will now use channel"; then + # shellcheck disable=SC2046 + echo $(red "failed refreshing charm: ${OUT}") + exit 5 + fi + # shellcheck disable=SC2059 + printf "${OUT}\n" + + wait_for "ubuntu" "$(charm_channel "ubuntu" "edge")" + wait_for "ubuntu" "$(idle_condition "ubuntu")" + + destroy_model "${model_name}" +} + test_switch() { if [ "$(skip 'test_switch')" ]; then echo "==> TEST SKIPPED: refresh switch" @@ -130,8 +194,10 @@ test_switch() { cd .. || exit run "run_refresh_switch_cs_to_ch" + run "run_refresh_switch_cs_to_ch_no_new_revision" run "run_refresh_switch_cs_to_ch_channel" run "run_refresh_switch_local_to_ch_channel" run "run_refresh_switch_channel" + run "run_refresh_switch_channel_no_new_revision" ) } From c33f2d98849bdddf3211f77b02365f4c6327ffa9 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Mon, 6 Feb 2023 15:05:26 -0500 Subject: [PATCH 07/10] Relocated 2 tests not using switch flag to refresh.sh --- tests/suites/refresh/refresh.sh | 59 +++++++++++++++++++++++++++++++++ tests/suites/refresh/switch.sh | 59 --------------------------------- 2 files changed, 59 insertions(+), 59 deletions(-) diff --git a/tests/suites/refresh/refresh.sh b/tests/suites/refresh/refresh.sh index ee41de8e514..52d101af11a 100644 --- a/tests/suites/refresh/refresh.sh +++ b/tests/suites/refresh/refresh.sh @@ -60,6 +60,63 @@ run_refresh_local() { destroy_model "${model_name}" } +run_refresh_channel() { + # Test juju refresh from one channel to another + echo + + model_name="test-refresh-switch-channel" + file="${TEST_DIR}/${model_name}.log" + + ensure "${model_name}" "${file}" + + juju deploy juju-qa-test + wait_for "juju-qa-test" "$(idle_condition "juju-qa-test")" + + OUT=$(juju refresh juju-qa-test --channel 2.0/edge 2>&1 || true) + # shellcheck disable=SC2059 + printf "${OUT}\n" + + # format: Added local charm "ubuntu", revision 2, to the model + revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') + + wait_for "juju-qa-test" "$(charm_rev "juju-qa-test" "${revision}")" + wait_for "juju-qa-test" "$(charm_channel "juju-qa-test" "2.0/edge")" + wait_for "juju-qa-test" "$(idle_condition "juju-qa-test")" + + destroy_model "${model_name}" +} + +run_refresh_channel_no_new_revision() { + # Test juju refresh from one channel to another, with no new + # charm revision. + echo + + model_name="test-refresh-switch-channel-no-new-revision" + file="${TEST_DIR}/${model_name}.log" + + ensure "${model_name}" "${file}" + + juju deploy ubuntu + wait_for "ubuntu" "$(idle_condition "ubuntu")" + + OUT=$(juju refresh ubuntu --channel edge 2>&1 || true) + # shellcheck disable=SC2059 + printf "${OUT}\n" + + if echo "${OUT}" | grep -E -vq "all future refreshes will now use channel"; then + # shellcheck disable=SC2046 + echo $(red "failed refreshing charm: ${OUT}") + exit 5 + fi + # shellcheck disable=SC2059 + printf "${OUT}\n" + + wait_for "ubuntu" "$(charm_channel "ubuntu" "edge")" + wait_for "ubuntu" "$(idle_condition "ubuntu")" + + destroy_model "${model_name}" +} + test_basic() { if [ "$(skip 'test_basic')" ]; then echo "==> TEST SKIPPED: basic refresh" @@ -73,5 +130,7 @@ test_basic() { run "run_refresh_cs" run "run_refresh_local" + run "run_refresh_channel" + run "run_refresh_channel_no_new_revision" ) } diff --git a/tests/suites/refresh/switch.sh b/tests/suites/refresh/switch.sh index 6cf99526d30..72ee60dedbc 100644 --- a/tests/suites/refresh/switch.sh +++ b/tests/suites/refresh/switch.sh @@ -125,63 +125,6 @@ run_refresh_switch_local_to_ch_channel() { destroy_model "${model_name}" } -run_refresh_switch_channel() { - # Test juju refresh switching from one channel to another - echo - - model_name="test-refresh-switch-channel" - file="${TEST_DIR}/${model_name}.log" - - ensure "${model_name}" "${file}" - - juju deploy juju-qa-test - wait_for "juju-qa-test" "$(idle_condition "juju-qa-test")" - - OUT=$(juju refresh juju-qa-test --channel 2.0/edge 2>&1 || true) - # shellcheck disable=SC2059 - printf "${OUT}\n" - - # format: Added local charm "ubuntu", revision 2, to the model - revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') - - wait_for "juju-qa-test" "$(charm_rev "juju-qa-test" "${revision}")" - wait_for "juju-qa-test" "$(charm_channel "juju-qa-test" "2.0/edge")" - wait_for "juju-qa-test" "$(idle_condition "juju-qa-test")" - - destroy_model "${model_name}" -} - -run_refresh_switch_channel_no_new_revision() { - # Test juju refresh switching from one channel to another, with no new - # charm revision. - echo - - model_name="test-refresh-switch-channel-no-new-revision" - file="${TEST_DIR}/${model_name}.log" - - ensure "${model_name}" "${file}" - - juju deploy ubuntu - wait_for "ubuntu" "$(idle_condition "ubuntu")" - - OUT=$(juju refresh ubuntu --channel edge 2>&1 || true) - # shellcheck disable=SC2059 - printf "${OUT}\n" - - if echo "${OUT}" | grep -E -vq "all future refreshes will now use channel"; then - # shellcheck disable=SC2046 - echo $(red "failed refreshing charm: ${OUT}") - exit 5 - fi - # shellcheck disable=SC2059 - printf "${OUT}\n" - - wait_for "ubuntu" "$(charm_channel "ubuntu" "edge")" - wait_for "ubuntu" "$(idle_condition "ubuntu")" - - destroy_model "${model_name}" -} - test_switch() { if [ "$(skip 'test_switch')" ]; then echo "==> TEST SKIPPED: refresh switch" @@ -197,7 +140,5 @@ test_switch() { run "run_refresh_switch_cs_to_ch_no_new_revision" run "run_refresh_switch_cs_to_ch_channel" run "run_refresh_switch_local_to_ch_channel" - run "run_refresh_switch_channel" - run "run_refresh_switch_channel_no_new_revision" ) } From 81cae86881f4199f9dbb4a5f55fef30b6ac01af1 Mon Sep 17 00:00:00 2001 From: Joseph Phillips Date: Tue, 7 Feb 2023 10:54:24 +0100 Subject: [PATCH 08/10] Changes sync-agent-binary to accept --agent-version instead of --version. The --version flag is not actually recognised, and agent-version is congruent with the bootstrap command. --- cmd/juju/commands/synctools.go | 8 ++++---- cmd/juju/commands/synctools_test.go | 17 ++++++++++------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/cmd/juju/commands/synctools.go b/cmd/juju/commands/synctools.go index acd08934cf4..676fac549f8 100644 --- a/cmd/juju/commands/synctools.go +++ b/cmd/juju/commands/synctools.go @@ -57,8 +57,8 @@ The online store will, of course, need to be contacted at some point to get the software. Examples: - juju sync-agent-binary --debug --version 2.0 - juju sync-agent-binary --debug --version 2.0 --local-dir=/home/ubuntu/sync-agent-binary + juju sync-agent-binary --debug --agent-version 2.0 + juju sync-agent-binary --debug --agent-version 2.0 --local-dir=/home/ubuntu/sync-agent-binary See also: upgrade-controller @@ -76,7 +76,7 @@ func (c *syncAgentBinaryCommand) Info() *cmd.Info { func (c *syncAgentBinaryCommand) SetFlags(f *gnuflag.FlagSet) { c.ModelCommandBase.SetFlags(f) - f.StringVar(&c.versionStr, "version", "", "Copy a specific major[.minor] version") + f.StringVar(&c.versionStr, "agent-version", "", "Copy a specific major[.minor] version") f.BoolVar(&c.dryRun, "dry-run", false, "Don't copy, just print what would be copied") f.BoolVar(&c.public, "public", false, "Tools are for a public cloud, so generate mirrors information") f.StringVar(&c.source, "source", "", "Local source directory") @@ -86,7 +86,7 @@ func (c *syncAgentBinaryCommand) SetFlags(f *gnuflag.FlagSet) { func (c *syncAgentBinaryCommand) Init(args []string) error { if c.versionStr == "" { - return errors.NewNotValid(nil, "--version is required") + return errors.NewNotValid(nil, "--agent-version is required") } var err error if c.targetVersion, err = version.Parse(c.versionStr); err != nil { diff --git a/cmd/juju/commands/synctools_test.go b/cmd/juju/commands/synctools_test.go index e32379ccfcc..02dabbd8fe5 100644 --- a/cmd/juju/commands/synctools_test.go +++ b/cmd/juju/commands/synctools_test.go @@ -73,21 +73,21 @@ type syncToolCommandTestCase struct { var syncToolCommandTests = []syncToolCommandTestCase{ { description: "minimum argument", - args: []string{"--version", "2.9.99", "-m", "test-target"}, + args: []string{"--agent-version", "2.9.99", "-m", "test-target"}, }, { description: "specifying also the synchronization source", - args: []string{"--version", "2.9.99", "-m", "test-target", "--source", "/foo/bar"}, + args: []string{"--agent-version", "2.9.99", "-m", "test-target", "--source", "/foo/bar"}, source: "/foo/bar", }, { description: "just make a dry run", - args: []string{"--version", "2.9.99", "-m", "test-target", "--dry-run"}, + args: []string{"--agent-version", "2.9.99", "-m", "test-target", "--dry-run"}, dryRun: true, }, { description: "specified public (ignored by API)", - args: []string{"--version", "2.9.99", "-m", "test-target", "--public"}, + args: []string{"--agent-version", "2.9.99", "-m", "test-target", "--public"}, public: false, }, } @@ -129,7 +129,8 @@ func (s *syncToolSuite) TestSyncToolsCommand(c *gc.C) { func (s *syncToolSuite) TestSyncToolsCommandTargetDirectory(c *gc.C) { dir := c.MkDir() - ctrl, run := s.getSyncAgentBinariesCommand(c, "--version", "2.9.99", "-m", "test-target", "--local-dir", dir, "--stream", "proposed") + ctrl, run := s.getSyncAgentBinariesCommand( + c, "--agent-version", "2.9.99", "-m", "test-target", "--local-dir", dir, "--stream", "proposed") defer ctrl.Finish() called := false @@ -155,7 +156,8 @@ func (s *syncToolSuite) TestSyncToolsCommandTargetDirectory(c *gc.C) { func (s *syncToolSuite) TestSyncToolsCommandTargetDirectoryPublic(c *gc.C) { dir := c.MkDir() - ctrl, run := s.getSyncAgentBinariesCommand(c, "--version", "2.9.99", "-m", "test-target", "--local-dir", dir, "--public") + ctrl, run := s.getSyncAgentBinariesCommand( + c, "--agent-version", "2.9.99", "-m", "test-target", "--local-dir", dir, "--public") defer ctrl.Finish() called := false @@ -187,7 +189,8 @@ func (s *syncToolSuite) TestAPIAdapterUploadTools(c *gc.C) { } func (s *syncToolSuite) TestAPIAdapterBlockUploadTools(c *gc.C) { - ctrl, run := s.getSyncAgentBinariesCommand(c, "-m", "test-target", "--version", "2.9.99", "--local-dir", c.MkDir(), "--stream", "released") + ctrl, run := s.getSyncAgentBinariesCommand( + c, "-m", "test-target", "--agent-version", "2.9.99", "--local-dir", c.MkDir(), "--stream", "released") defer ctrl.Finish() syncTools = func(sctx *sync.SyncContext) error { From c4a1a64b453f1cda83dc53fc49596caf4bcf78b7 Mon Sep 17 00:00:00 2001 From: Jack Shaw Date: Wed, 1 Feb 2023 11:24:24 +0000 Subject: [PATCH 09/10] Clean up entity parsing for debug-log Show warnings when filters filter non log giving sources Also remove unsupported feature where debug-log filtering allows filtering by regex, but only sometimes. Now only raw strings or wildcards are supported Do this by escaping regex then unescaping the wildcard when querying Mongo. --- api/common/logs.go | 4 +- cmd/juju/commands/debuglog.go | 77 +++++++++++++++++++---------------- state/logs.go | 6 ++- 3 files changed, 48 insertions(+), 39 deletions(-) diff --git a/api/common/logs.go b/api/common/logs.go index da9abf7cabe..b707f33047c 100644 --- a/api/common/logs.go +++ b/api/common/logs.go @@ -22,7 +22,7 @@ import ( // closes the connection. type DebugLogParams struct { // IncludeEntity lists entity tags to include in the response. Tags may - // finish with a '*' to match a prefix e.g.: unit-mysql-*, machine-2. If + // include '*' wildcards e.g.: unit-mysql-*, machine-2. If // none are set, then all lines are considered included. IncludeEntity []string // IncludeModule lists logging modules to include in the response. If none @@ -33,7 +33,7 @@ type DebugLogParams struct { // are set all labels are considered included. IncludeLabel []string // ExcludeEntity lists entity tags to exclude from the response. As with - // IncludeEntity the values may finish with a '*'. + // IncludeEntity the values may include '*' wildcards. ExcludeEntity []string // ExcludeModule lists logging modules to exclude from the response. If a // module is specified, all the submodules are also excluded. diff --git a/cmd/juju/commands/debuglog.go b/cmd/juju/commands/debuglog.go index 302e024ba0f..b134e95deed 100644 --- a/cmd/juju/commands/debuglog.go +++ b/cmd/juju/commands/debuglog.go @@ -11,6 +11,7 @@ import ( "github.com/juju/ansiterm" "github.com/juju/cmd/v3" + "github.com/juju/collections/transform" "github.com/juju/errors" "github.com/juju/gnuflag" "github.com/juju/juju/jujuclient" @@ -46,7 +47,8 @@ machines and units can be seen in the output of `[1:] + "`juju status`" + `. The '--include' and '--exclude' options filter by entity. The entity can be a machine, unit, or application for vm models, but can be application only -for k8s models. +for k8s models. These filters support wildcards ` + "`*`" + ` if filtering on the +entity full name (prefixed by ` + "`-`" + `) The '--include-module' and '--exclude-module' options filter by (dotted) logging module name. The module name can be truncated such that all loggers @@ -204,45 +206,50 @@ func (c *debugLogCommand) Init(args []string) error { return errors.Trace(err) } isCaas := modelType == model.CAAS - c.params.IncludeEntity = c.processEntities(isCaas, c.params.IncludeEntity) - c.params.ExcludeEntity = c.processEntities(isCaas, c.params.ExcludeEntity) + if isCaas { + c.params.IncludeEntity = transform.Slice(c.params.IncludeEntity, c.parseCAASEntity) + c.params.ExcludeEntity = transform.Slice(c.params.ExcludeEntity, c.parseCAASEntity) + } else { + c.params.IncludeEntity = transform.Slice(c.params.IncludeEntity, c.parseEntity) + c.params.ExcludeEntity = transform.Slice(c.params.ExcludeEntity, c.parseEntity) + } return cmd.CheckEmpty(args) } -func (c *debugLogCommand) processEntities(isCAAS bool, entities []string) []string { - if entities == nil { - return nil +func (c *debugLogCommand) parseEntity(entity string) string { + tag, err := names.ParseTag(entity) + switch { + case strings.Contains(entity, "*"): + return entity + case err == nil && (tag.Kind() == names.ApplicationTagKind || tag.Kind() == names.MachineTagKind || tag.Kind() == names.UnitTagKind): + return tag.String() + case names.IsValidMachine(entity): + return names.NewMachineTag(entity).String() + case names.IsValidUnit(entity): + return names.NewUnitTag(entity).String() + case names.IsValidApplication(entity): + // If the user asks for --include nova-compute, we should give all + // nova-compute units for IAAS models. + return names.UnitTagKind + "-" + entity + "-*" + default: + logger.Warningf("%q was not recognised as a valid application, machine or unit name", entity) + return entity } - result := make([]string, len(entities)) - for i, entity := range entities { - // A stringified unit or machine tag never match their "IsValid" - // function from names, so if the string value passed in is a valid - // machine or unit, then convert here. - if names.IsValidMachine(entity) { - entity = names.NewMachineTag(entity).String() - } else if names.IsValidUnit(entity) { - entity = names.NewUnitTag(entity).String() - } else { - // Now we want to deal with a special case. Both stringified - // machine tags and stringified units are valid application names. - // So here we use special knowledge about how tags are serialized to - // be able to give a better user experience. If the user asks for - // --include nova-compute, we should give all nova-compute units. - if strings.HasPrefix(entity, names.UnitTagKind+"-") || - strings.HasPrefix(entity, names.MachineTagKind+"-") { - // no-op pass through - } else if names.IsValidApplication(entity) { - // Assume that the entity refers to an application. - if isCAAS { - entity = names.NewApplicationTag(entity).String() - } else { - entity = names.UnitTagKind + "-" + entity + "-*" - } - } - } - result[i] = entity +} + +func (c *debugLogCommand) parseCAASEntity(entity string) string { + tag, err := names.ParseTag(entity) + switch { + case strings.Contains(entity, "*"): + return entity + case err == nil && tag.Kind() == names.ApplicationTagKind: + return tag.String() + case names.IsValidApplication(entity): + return names.NewApplicationTag(entity).String() + default: + logger.Warningf("%q was not recognised as a valid application name. Only applications produce logs for CAAS models application", entity) + return entity } - return result } type DebugLogAPI interface { diff --git a/state/logs.go b/state/logs.go index b0945679f4b..45585fb77da 100644 --- a/state/logs.go +++ b/state/logs.go @@ -797,8 +797,10 @@ func makeEntityPattern(entities []string) string { var patterns []string for _, entity := range entities { // Convert * wildcard to the regex equivalent. This is safe - // because * never appears in entity names. - patterns = append(patterns, strings.Replace(entity, "*", ".*", -1)) + // because * never appears in entity names. Escape any other regex. + escaped := regexp.QuoteMeta(entity) + unescapedWildcards := strings.Replace(escaped, regexp.QuoteMeta("*"), ".*", -1) + patterns = append(patterns, unescapedWildcards) } return `^(` + strings.Join(patterns, "|") + `)$` } From 823cf425ee75b185a80b00e20d55bab9f3190748 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Mon, 6 Feb 2023 15:41:05 -0500 Subject: [PATCH 10/10] Update refresh integration tests. Add small tweeks to tests including fixing model names, correcting a charm name and enhancing comments. --- tests/suites/refresh/refresh.sh | 10 +++++----- tests/suites/refresh/switch.sh | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/suites/refresh/refresh.sh b/tests/suites/refresh/refresh.sh index 52d101af11a..29214bab4e8 100644 --- a/tests/suites/refresh/refresh.sh +++ b/tests/suites/refresh/refresh.sh @@ -19,7 +19,7 @@ run_refresh_cs() { # shellcheck disable=SC2059 printf "${OUT}\n" - # Added charm-store charm "ubuntu", revision 21 in channel stable, to the model + # format: Added charm-store charm "ubuntu", revision 21 in channel stable, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "ubuntu" "$(charm_rev "ubuntu" "${revision}")" @@ -51,7 +51,7 @@ run_refresh_local() { # shellcheck disable=SC2059 printf "${OUT}\n" - # Added local charm "ubuntu", revision 2, to the model + # format: Added charm-store charm "ubuntu", revision 21 in channel stable, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "ubuntu" "$(charm_rev "ubuntu" "${revision}")" @@ -64,7 +64,7 @@ run_refresh_channel() { # Test juju refresh from one channel to another echo - model_name="test-refresh-switch-channel" + model_name="test-refresh-channel" file="${TEST_DIR}/${model_name}.log" ensure "${model_name}" "${file}" @@ -76,7 +76,7 @@ run_refresh_channel() { # shellcheck disable=SC2059 printf "${OUT}\n" - # format: Added local charm "ubuntu", revision 2, to the model + # format: Added charm-store charm "ubuntu", revision 21 in channel stable, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "juju-qa-test" "$(charm_rev "juju-qa-test" "${revision}")" @@ -91,7 +91,7 @@ run_refresh_channel_no_new_revision() { # charm revision. echo - model_name="test-refresh-switch-channel-no-new-revision" + model_name="test-refresh-channel-no-new-revision" file="${TEST_DIR}/${model_name}.log" ensure "${model_name}" "${file}" diff --git a/tests/suites/refresh/switch.sh b/tests/suites/refresh/switch.sh index 72ee60dedbc..76088fba237 100644 --- a/tests/suites/refresh/switch.sh +++ b/tests/suites/refresh/switch.sh @@ -19,7 +19,7 @@ run_refresh_switch_cs_to_ch() { # shellcheck disable=SC2059 printf "${OUT}\n" - # format: Added local charm "ubuntu", revision 2, to the model + # format: Added charm-store charm "ubuntu", revision 21 in channel stable, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "ubuntu" "$(charm_rev "ubuntu" "${revision}")" @@ -33,15 +33,15 @@ run_refresh_switch_cs_to_ch_no_new_revision() { # charm revision. echo - model_name="test-refresh-switch-ch" + model_name="test-refresh-switch-ch-no-new-revision" file="${TEST_DIR}/${model_name}.log" ensure "${model_name}" "${file}" - OUT=$(juju deploy cs:ubuntu2 >&1 || true) + OUT=$(juju deploy cs:ubuntu >&1 || true) # shellcheck disable=SC2059 printf "${OUT}\n" - # format: Added local charm "ubuntu", revision 2, to the model + # format: Added charm-store charm "ubuntu", revision 21 in channel stable, to the model cs_revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "ubuntu" "$(idle_condition "ubuntu")" @@ -82,7 +82,7 @@ run_refresh_switch_cs_to_ch_channel() { # shellcheck disable=SC2059 printf "${OUT}\n" - # format: Added local charm "ubuntu", revision 2, to the model + # format: Added charm-store charm "ubuntu", revision 21 in channel stable, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "ubuntu" "$(charm_rev "ubuntu" "${revision}")" @@ -115,7 +115,7 @@ run_refresh_switch_local_to_ch_channel() { # shellcheck disable=SC2059 printf "${OUT}\n" - # format: Added local charm "ubuntu", revision 2, to the model + # format: Added charm-store charm "ubuntu", revision 21 in channel stable, to the model revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') wait_for "ubuntu" "$(charm_rev "ubuntu" "${revision}")"