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

[v8] Max in flight flag #3085

Merged
merged 13 commits into from
Aug 9, 2024
Merged
2 changes: 1 addition & 1 deletion actor/v7pushaction/actor.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func NewActor(v3Actor V7Actor, sharedActor SharedActor) *Actor {
SetDefaultBitsPathForPushPlan,
SetupDropletPathForPushPlan,
actor.SetupAllResourcesForPushPlan,
SetupDeploymentStrategyForPushPlan,
SetupDeploymentInformationForPushPlan,
SetupNoStartForPushPlan,
SetupNoWaitForPushPlan,
SetupTaskAppForPushPlan,
Expand Down
2 changes: 1 addition & 1 deletion actor/v7pushaction/actor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ var _ = Describe("Actor", func() {
SetDefaultBitsPathForPushPlan,
SetupDropletPathForPushPlan,
actor.SetupAllResourcesForPushPlan,
SetupDeploymentStrategyForPushPlan,
SetupDeploymentInformationForPushPlan,
SetupNoStartForPushPlan,
SetupNoWaitForPushPlan,
SetupTaskAppForPushPlan,
Expand Down
14 changes: 10 additions & 4 deletions actor/v7pushaction/create_deployment_for_push_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,16 @@ import (
func (actor Actor) CreateDeploymentForApplication(pushPlan PushPlan, eventStream chan<- *PushEvent, progressBar ProgressBar) (PushPlan, Warnings, error) {
eventStream <- &PushEvent{Plan: pushPlan, Event: StartingDeployment}

var dep resources.Deployment
dep.DropletGUID = pushPlan.DropletGUID
dep.Strategy = pushPlan.Strategy
dep.Relationships = resources.Relationships{constant.RelationshipTypeApplication: resources.Relationship{GUID: pushPlan.Application.GUID}}
dep := resources.Deployment{
Copy link
Contributor

Choose a reason for hiding this comment

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

[comment, no action] good improvement / we should be using this style. I wonder if our linter catches the assignment approach you've replaced

Strategy: pushPlan.Strategy,
DropletGUID: pushPlan.DropletGUID,
Relationships: resources.Relationships{constant.RelationshipTypeApplication: resources.Relationship{GUID: pushPlan.Application.GUID}},
}

if pushPlan.MaxInFlight != 0 {
dep.Options = resources.DeploymentOpts{MaxInFlight: pushPlan.MaxInFlight}
}

deploymentGUID, warnings, err := actor.V7Actor.CreateDeployment(dep)

if err != nil {
Expand Down
46 changes: 46 additions & 0 deletions actor/v7pushaction/create_deployment_for_push_plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"code.cloudfoundry.org/cli/actor/v7action"
. "code.cloudfoundry.org/cli/actor/v7pushaction"
"code.cloudfoundry.org/cli/actor/v7pushaction/v7pushactionfakes"
"code.cloudfoundry.org/cli/api/cloudcontroller/ccv3/constant"
"code.cloudfoundry.org/cli/resources"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -106,6 +107,51 @@ var _ = Describe("CreateDeploymentForApplication()", func() {
Expect(events).To(ConsistOf(StartingDeployment))
})
})

When("strategy is provided", func() {
BeforeEach(func() {
fakeV7Actor.PollStartForDeploymentCalls(func(_ resources.Application, _ string, _ bool, handleInstanceDetails func(string)) (warnings v7action.Warnings, err error) {
handleInstanceDetails("Instances starting...")
return nil, nil
})

fakeV7Actor.CreateDeploymentReturns(
"some-deployment-guid",
v7action.Warnings{"some-deployment-warning"},
nil,
)
paramPlan.Strategy = "rolling"
paramPlan.MaxInFlight = 10
})

It("waits for the app to start", func() {
Expect(fakeV7Actor.PollStartForDeploymentCallCount()).To(Equal(1))
givenApp, givenDeploymentGUID, noWait, _ := fakeV7Actor.PollStartForDeploymentArgsForCall(0)
Expect(givenApp).To(Equal(resources.Application{GUID: "some-app-guid"}))
Expect(givenDeploymentGUID).To(Equal("some-deployment-guid"))
Expect(noWait).To(Equal(false))
Expect(events).To(ConsistOf(StartingDeployment, InstanceDetails, WaitingForDeployment))
Expect(fakeV7Actor.CreateDeploymentCallCount()).To(Equal(1))
dep := fakeV7Actor.CreateDeploymentArgsForCall(0)
Expect(dep).To(Equal(resources.Deployment{
Strategy: "rolling",
Options: resources.DeploymentOpts{MaxInFlight: 10},
Relationships: resources.Relationships{
constant.RelationshipTypeApplication: resources.Relationship{GUID: "some-app-guid"},
},
}))
})

It("returns errors and warnings", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

[suggestion, nonblocking] do we want a happy-path case covered?

Expect(returnedPushPlan).To(Equal(paramPlan))
Expect(executeErr).NotTo(HaveOccurred())
Expect(warnings).To(ConsistOf("some-deployment-warning"))
})

It("records deployment events", func() {
Expect(events).To(ConsistOf(StartingDeployment, InstanceDetails, WaitingForDeployment))
})
})
})

Describe("waiting for app to start", func() {
Expand Down
2 changes: 2 additions & 0 deletions actor/v7pushaction/push_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type PushPlan struct {
NoStart bool
NoWait bool
Strategy constant.DeploymentStrategy
MaxInFlight int
TaskTypeApplication bool

DockerImageCredentials v7action.DockerImageCredentials
Expand Down Expand Up @@ -47,6 +48,7 @@ type FlagOverrides struct {
HealthCheckType constant.HealthCheckType
Instances types.NullInt
Memory string
MaxInFlight *int
NoStart bool
NoWait bool
ProvidedAppPath string
Expand Down
13 changes: 13 additions & 0 deletions actor/v7pushaction/setup_deployment_information_for_push_plan.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package v7pushaction

import "code.cloudfoundry.org/cli/api/cloudcontroller/ccv3/constant"

func SetupDeploymentInformationForPushPlan(pushPlan PushPlan, overrides FlagOverrides) (PushPlan, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[suggestion / question / nonblocking] why is this separate from create_deployment_for_push_plan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actor.PreparePushPlanSequence looked like the place where we were configuring the push plan by translating all the needed flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay makes sense (the pushplansequence). In that case what's strangely-shaped is the location (siblings within v7pushaction) of two deployment functions that both take pushPlans. I see that this parallels the situation for e.g. droplet config, so I'm happy to move on from this topic for now and leave it up to a separate refactor

pushPlan.Strategy = overrides.Strategy

if overrides.Strategy != constant.DeploymentStrategyDefault && overrides.MaxInFlight != nil {
pushPlan.MaxInFlight = *overrides.MaxInFlight
}

return pushPlan, nil
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
. "github.com/onsi/gomega"
)

var _ = Describe("SetupDeploymentStrategyForPushPlan", func() {
var _ = Describe("SetupDeploymentInformationForPushPlan", func() {
var (
pushPlan PushPlan
overrides FlagOverrides
Expand All @@ -24,24 +24,47 @@ var _ = Describe("SetupDeploymentStrategyForPushPlan", func() {
})

JustBeforeEach(func() {
expectedPushPlan, executeErr = SetupDeploymentStrategyForPushPlan(pushPlan, overrides)
expectedPushPlan, executeErr = SetupDeploymentInformationForPushPlan(pushPlan, overrides)
})

When("flag overrides specifies strategy", func() {
BeforeEach(func() {
overrides.Strategy = "rolling"
maxInFlight := 5
overrides.MaxInFlight = &maxInFlight
})

It("sets the strategy on the push plan", func() {
Expect(executeErr).ToNot(HaveOccurred())
Expect(expectedPushPlan.Strategy).To(Equal(constant.DeploymentStrategyRolling))
})

It("sets the max in flight on the push plan", func() {
Expect(executeErr).ToNot(HaveOccurred())
Expect(expectedPushPlan.MaxInFlight).To(Equal(5))
})
})

When("flag overrides does not specify strategy", func() {
BeforeEach(func() {
maxInFlight := 10
overrides.MaxInFlight = &maxInFlight
})
It("leaves the strategy as its default value on the push plan", func() {
Expect(executeErr).ToNot(HaveOccurred())
Expect(expectedPushPlan.Strategy).To(Equal(constant.DeploymentStrategyDefault))
})

It("does not set MaxInFlight", func() {
Expect(executeErr).ToNot(HaveOccurred())
Expect(expectedPushPlan.MaxInFlight).To(Equal(0))
})
})

When("flag not provided", func() {
It("does not set MaxInFlight", func() {
Expect(executeErr).ToNot(HaveOccurred())
Expect(expectedPushPlan.MaxInFlight).To(Equal(0))
})
})
})
7 changes: 0 additions & 7 deletions actor/v7pushaction/setup_deployment_strategy_for_push_plan.go

This file was deleted.

7 changes: 7 additions & 0 deletions command/translatableerror/convert_to_translatable_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,13 @@ func ConvertToTranslatableError(err error) error {
return RunTaskError{Message: "App is not staged."}
}

if strings.Contains(e.Message, "Unknown field(s): 'options'") {
return MinimumCFAPIVersionNotMetError{
Command: "'--max-in-flight' flag",
MinimumVersion: "3.173.0",
}
}

// JSON Errors
case *json.SyntaxError:
return JSONSyntaxError{Err: e}
Expand Down
18 changes: 16 additions & 2 deletions command/v7/copy_source_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type CopySourceCommand struct {
RequiredArgs flag.CopySourceArgs `positional-args:"yes"`
usage interface{} `usage:"CF_NAME copy-source SOURCE_APP DESTINATION_APP [-s TARGET_SPACE [-o TARGET_ORG]] [--no-restart] [--strategy STRATEGY] [--no-wait]"`
Strategy flag.DeploymentStrategy `long:"strategy" description:"Deployment strategy can be canary, rolling or null"`
MaxInFlight *int `long:"max-in-flight" description:"Defines the maximum number of instances that will be actively being started. Only applies when --strategy flag is specified."`
NoWait bool `long:"no-wait" description:"Exit when the first instance of the web process is healthy"`
NoRestart bool `long:"no-restart" description:"Do not restage the destination application"`
Organization string `short:"o" long:"organization" description:"Org that contains the destination application"`
Expand Down Expand Up @@ -48,6 +49,14 @@ func (cmd *CopySourceCommand) ValidateFlags() error {
}
}

if cmd.Strategy.Name == constant.DeploymentStrategyDefault && cmd.MaxInFlight != nil {
Copy link
Contributor

@pivotalgeorge pivotalgeorge Aug 9, 2024

Choose a reason for hiding this comment

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

[suggestion, blocking] I believe this is now incorrect: MaxInFlight can take a value of 0, which should be invalid for this flag
edit: I was looking at a diff against the previous changeset version of this PR, so missed the block below. So, disregard the positive value check comment.
However, shouldn't this read && cmd.MaxInFlight == nil, as a required-arg check
-> I see that the default deployment strategy is empty-string, so this is actually correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

return translatableerror.RequiredFlagsError{Arg1: "--max-in-flight", Arg2: "--strategy"}
}

if cmd.Strategy.Name != constant.DeploymentStrategyDefault && cmd.MaxInFlight != nil && *cmd.MaxInFlight < 1 {
return translatableerror.IncorrectUsageError{Message: "--max-in-flight must be greater than or equal to 1"}
}

return nil
}

Expand Down Expand Up @@ -160,10 +169,15 @@ func (cmd CopySourceCommand) Execute(args []string) error {
cmd.UI.DisplayNewline()

opts := shared.AppStartOpts{
Strategy: cmd.Strategy.Name,
NoWait: cmd.NoWait,
AppAction: constant.ApplicationRestarting,
NoWait: cmd.NoWait,
Strategy: cmd.Strategy.Name,
}

if cmd.MaxInFlight != nil {
opts.MaxInFlight = *cmd.MaxInFlight
}

err = cmd.Stager.StageAndStart(targetApp, targetSpace, targetOrg, pkg.GUID, opts)
if err != nil {
return mapErr(cmd.Config, targetApp.Name, err)
Expand Down
114 changes: 69 additions & 45 deletions command/v7/copy_source_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,49 +125,6 @@ var _ = Describe("copy-source Command", func() {
})
})

When("the target organization is specified but the targeted space isn't", func() {
BeforeEach(func() {
cmd.Organization = "some-other-organization"
})

It("returns an error", func() {
Expect(executeErr).To(MatchError(translatableerror.RequiredFlagsError{
Arg1: "--organization, -o",
Arg2: "--space, -s",
}))
})
})

When("the no restart and strategy flags are both provided", func() {
BeforeEach(func() {
cmd.NoRestart = true
cmd.Strategy = flag.DeploymentStrategy{Name: constant.DeploymentStrategyRolling}
})

It("returns an error", func() {
Expect(executeErr).To(MatchError(translatableerror.ArgumentCombinationError{
Args: []string{
"--no-restart", "--strategy",
},
}))
})
})

When("the no restart and no wait flags are both provided", func() {
BeforeEach(func() {
cmd.NoRestart = true
cmd.NoWait = true
})

It("returns an error", func() {
Expect(executeErr).To(MatchError(translatableerror.ArgumentCombinationError{
Args: []string{
"--no-restart", "--no-wait",
},
}))
})
})

When("a target org and space is provided", func() {
BeforeEach(func() {
cmd.Organization = "destination-org"
Expand Down Expand Up @@ -329,6 +286,8 @@ var _ = Describe("copy-source Command", func() {
cmd.Strategy = flag.DeploymentStrategy{
Name: constant.DeploymentStrategyRolling,
}
maxInFlight := 5
cmd.MaxInFlight = &maxInFlight
})

It("stages and starts the app with the appropriate strategy", func() {
Expand All @@ -338,9 +297,10 @@ var _ = Describe("copy-source Command", func() {
Expect(spaceForApp).To(Equal(configv3.Space{Name: "some-space", GUID: "some-space-guid"}))
Expect(orgForApp).To(Equal(configv3.Organization{Name: "some-org"}))
Expect(pkgGUID).To(Equal("target-package-guid"))
Expect(opts.Strategy).To(Equal(constant.DeploymentStrategyRolling))
Expect(opts.NoWait).To(Equal(false))
Expect(opts.AppAction).To(Equal(constant.ApplicationRestarting))
Expect(opts.MaxInFlight).To(Equal(5))
Expect(opts.NoWait).To(Equal(false))
Expect(opts.Strategy).To(Equal(constant.DeploymentStrategyRolling))
})
})

Expand All @@ -349,6 +309,8 @@ var _ = Describe("copy-source Command", func() {
cmd.Strategy = flag.DeploymentStrategy{
Name: constant.DeploymentStrategyCanary,
}
maxInFlight := 1
cmd.MaxInFlight = &maxInFlight
})

It("stages and starts the app with the appropriate strategy", func() {
Expand Down Expand Up @@ -417,4 +379,66 @@ var _ = Describe("copy-source Command", func() {
It("succeeds", func() {
Expect(executeErr).To(Not(HaveOccurred()))
})

DescribeTable("ValidateFlags returns an error",
func(setup func(), expectedErr error) {
setup()
err := cmd.ValidateFlags()
if expectedErr == nil {
Expect(err).To(BeNil())
} else {
Expect(err).To(MatchError(expectedErr))
}
},
Entry("the target organization is specified but the targeted space isn't",
func() {
cmd.Organization = "some-other-organization"
},
translatableerror.RequiredFlagsError{
Arg1: "--organization, -o",
Arg2: "--space, -s",
}),

Entry("the no restart and strategy flags are both provided",
func() {
cmd.NoRestart = true
cmd.Strategy = flag.DeploymentStrategy{Name: constant.DeploymentStrategyRolling}
},
translatableerror.ArgumentCombinationError{
Args: []string{
"--no-restart", "--strategy",
},
}),

Entry("the no restart and no wait flags are both provided",
func() {
cmd.NoRestart = true
cmd.NoWait = true
},
translatableerror.ArgumentCombinationError{
Args: []string{
"--no-restart", "--no-wait",
},
}),

Entry("max-in-flight is passed without strategy",
func() {
maxInFlight := 5
cmd.MaxInFlight = &maxInFlight
},
translatableerror.RequiredFlagsError{
Arg1: "--max-in-flight",
Arg2: "--strategy",
}),

Entry("max-in-flight is smaller than 1",
func() {
cmd.Strategy = flag.DeploymentStrategy{Name: constant.DeploymentStrategyRolling}
maxInFlight := 0
cmd.MaxInFlight = &maxInFlight
},
translatableerror.IncorrectUsageError{
Message: "--max-in-flight must be greater than or equal to 1",
}),
)
})
Loading
Loading