[specific ci=Group11-Upgrade] Add UpgradeInProgress flag for VCH upgrade by chengwang86 · Pull Request #4847 · vmware/vic

chengwang86 · 2017-04-21T23:32:33Z

This PR adds a flag UpgradeInProgress to the VCH's ExtraConfig.

during VCH upgrade, if UpgradeInProgress==true, the current upgrade operation will fail since there might be another upgrade operation running.
I also add vic-machine upgrade --resetInProgressFlag so that the user could reset the flag to false in case the flag is polluted by previous operations that do not finish properly.
test cases are added

emlin · 2017-04-24T14:05:04Z

cmd/vic-machine/inspect/inspect.go

+	upgrading, err := vch.GetVCHUpdateStatus(ctx)
 	if err != nil {
-		log.Errorf("Unable to determine if upgrade is in progress: %s", err)
+		log.Errorf("Unable to determine if upgrade/update is in progress: %s", err)


s/update/configure/g

emlin · 2017-04-24T14:05:14Z

cmd/vic-machine/inspect/inspect.go

 	}
 	if upgrading {
-		log.Info("Upgrade in progress")
+		log.Info("Upgrade/update in progress")


emlin · 2017-04-24T14:14:19Z

cmd/vic-machine/upgrade/upgrade.go

 		},
+		cli.BoolFlag{
+			Name:        "resetInProgressFlag",
+			Usage:       "Reset the UpdateInProgress flag",


This is upgrade command, name the flag as UpgradeInProgress.

emlin · 2017-04-24T14:16:13Z

cmd/vic-machine/upgrade/upgrade.go

+		return errors.New("upgrade failed")
+	}
+	if upgrading {
+		log.Error("Upgrade failed: another upgrade/update operation is in progress")


Please mention how to fix it using option --resetInProgressFlag

emlin · 2017-04-24T14:25:10Z

pkg/vsphere/vm/vm.go

 }
+
+// GetVCHUpdateStatus tells if an upgrade/update has already been started based on the UpdateInProgress flag in ExtraConfig
+// If error != nil, VCH upgrade/update will exit; so the value of the VCH upgrade/update status is not important


method help should not talk about the behavior of callers
here should describe while any error happens in vm operation, will return error

emlin · 2017-04-24T14:29:18Z

pkg/vsphere/vm/vm.go

+	if v, ok := info[UpdateInProgress]; ok {
+		updateStatus, err := strconv.ParseBool(v)
+		if err != nil {
+			//  If error occurs, return true to cancel VCH upgrade. The user can reset the UpdateInProgress flag later.


if error happens, the bool return value should not matter for the caller.
again do not comment caller's behavior

mhagen-vmware

couple small changes, otherwise lgtm

mhagen-vmware · 2017-04-24T14:30:10Z

tests/test-cases/Group11-Upgrade/11-04-Upgrade-UpdateInProgress.robot

+    [Arguments]  ${expected}
+    ${rc}  ${output}=  Run And Return Rc And Output  bin/vic-machine-linux inspect --name=%{VCH-NAME} --target=%{TEST_URL} --thumbprint=%{TEST_THUMBPRINT} --user=%{TEST_USERNAME} --password=%{TEST_PASSWORD} --compute-resource=%{TEST_RESOURCE}
+    Should Be Equal As Integers  ${rc}  0
+    Should Contain  ${output}  ${expected}


Pull this into VCH-Util.robot please.

mhagen-vmware · 2017-04-24T14:30:47Z

tests/test-cases/Group11-Upgrade/11-04-Upgrade-UpdateInProgress.robot

+    [Arguments]  ${expected}
+    ${rc}  ${output}=  Run And Return Rc And Output  govc vm.info -e %{VCH-NAME} | grep UpdateInProgress
+    Should Be Equal As Integers  ${rc}  0
+    Should Contain  ${output}  ${expected}


this one too

emlin · 2017-04-24T14:31:56Z

pkg/vsphere/vm/vm.go

+
+// GetVCHUpdateStatus tells if an upgrade/update has already been started based on the UpdateInProgress flag in ExtraConfig
+// If error != nil, VCH upgrade/update will exit; so the value of the VCH upgrade/update status is not important
+func (vm *VirtualMachine) GetVCHUpdateStatus(ctx context.Context) (bool, error) {


GetVCHUpdateStatus => VCHUpdateStatus

emlin · 2017-04-24T14:32:27Z

pkg/vsphere/vm/vm.go

+}
+
+// SetVCHUpdateStatus sets the "UpdateInProgress" flag in ExtraConfig
+func (vm *VirtualMachine) SetVCHUpdateStatus(ctx context.Context, updateStatus string) error {


get set method should use same type of value, all string or bool

emlin · 2017-04-24T15:53:12Z

cmd/vic-machine/upgrade/upgrade.go

 	if upgrading {
-		log.Error("Upgrade failed: another upgrade/update operation is in progress")
+		log.Error("Upgrade failed: another upgrade/configure operation is in progress")
+		log.Error("Use --resetInProgressFlag to reset the VCH upgrade/configure status")


still need to mention, if there is no other upgrade or configure process running, use ...

emlin · 2017-04-24T15:54:38Z

pkg/vsphere/vm/vm.go

-func (vm *VirtualMachine) GetVCHUpdateStatus(ctx context.Context) (bool, error) {
+// VCHUpgradeStatus tells if an upgrade/configure has already been started based on the UpgradeInProgress flag in ExtraConfig
+// It returns the error if the vm operation does not succeed
+func (vm *VirtualMachine) VCHUpgradeStatus(ctx context.Context) (bool, error) {


this method will be reused by upgrade and configure, so might still use update as name

please also add unit test for these two methods

emlin · 2017-04-24T15:56:47Z

cmd/vic-machine/upgrade/upgrade.go

 		},
+		cli.BoolFlag{
+			Name:        "resetInProgressFlag",
+			Usage:       "Reset the UpgradeInProgress flag",


feels like here should have a warning message as well. to make sure user does not reset flag during other process is actually running

emlin · 2017-04-24T18:29:47Z

pkg/vsphere/vm/vm.go

+		updateStatus, err := strconv.ParseBool(v)
 		if err != nil {
 			//  If error occurs, the bool return value does not matter for the caller.
 			return true, fmt.Errorf("failed to parse %s to bool: %s", v, err)


I think here should return false

emlin · 2017-04-24T18:30:51Z

pkg/vsphere/vm/vm.go

 		return false, err
 	}

 	if v, ok := info[UpgradeInProgress]; ok {


name is still inconsistent

mhagen-vmware

lgtm

…Config

…ade (vmware#4847)

vmwclabot added the cla-not-required label Apr 21, 2017

chengwang86 requested review from emlin and mhagen-vmware April 24, 2017 05:12

emlin reviewed Apr 24, 2017

View reviewed changes

mhagen-vmware suggested changes Apr 24, 2017

View reviewed changes

emlin reviewed Apr 24, 2017

View reviewed changes

chengwang86 changed the title ~~[specific ci=Group11-Upgrade] Add UpdateInProgress flag for VCH upgrade~~ [specific ci=Group11-Upgrade] Add UpgradeInProgress flag for VCH upgrade Apr 24, 2017

emlin reviewed Apr 24, 2017

View reviewed changes

pkg/vsphere/vm/vm.go Outdated

return false, err

}

if v, ok := info[UpgradeInProgress]; ok {

Copy link

Contributor

emlin Apr 24, 2017

Choose a reason for hiding this comment

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

name is still inconsistent

mhagen-vmware approved these changes Apr 24, 2017

View reviewed changes

emlin approved these changes Apr 24, 2017

View reviewed changes

chengwang86 added 10 commits April 25, 2017 07:15

detect VCH update/upgrade status by setting UpdateInProgress in Extra…

f4494b9

…Config

add test cases

900101b

update tests

4040a12

fix gofmt error

32fae61

update test case 11-04

cc3b27b

update tests

99b4905

update test

2aa03a5

update tests and log messages

a64d5b6

added unit tests

bc53055

cosmetic change

ddb5278

cosmetic changes

43925ad

chengwang86 force-pushed the cheng-detectupgrade-4069 branch from e074906 to 43925ad Compare April 25, 2017 14:16

chengwang86 merged commit 3d35d84 into vmware:master Apr 25, 2017

matthewavery pushed a commit to matthewavery/vic that referenced this pull request May 3, 2017

[specific ci=Group11-Upgrade] Add UpgradeInProgress flag for VCH upgr…

fae35fe

…ade (vmware#4847)

zjs mentioned this pull request Sep 21, 2017

[skip ci] Define REST API handlers for inspecting VCHs #6318

Merged

Conversation

chengwang86 commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emlin Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhagen-vmware left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhagen-vmware left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chengwang86 commented Apr 21, 2017 •

edited

Loading

emlin Apr 24, 2017 •

edited

Loading