-
Notifications
You must be signed in to change notification settings - Fork 174
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
[specific ci=Group11-Upgrade] Reconfigure VM display name #4134
[specific ci=Group11-Upgrade] Reconfigure VM display name #4134
Conversation
e6f2118
to
8e94fc7
Compare
9304605
to
d434e9a
Compare
lib/config/executor/container_vm.go
Outdated
@@ -44,6 +44,21 @@ type Common struct { | |||
Notes string `vic:"0.1" scope:"hidden" key:"notes"` | |||
} | |||
|
|||
// Common data (specifically for a containerVM) between managed entities, across execution environments. | |||
type Common2 struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the thought process behind naming this Common2, I just want to be sure that there isn't a better or more appropriate name for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgairo We want to persist the name
in Common while the containeVM is still running, so we need to set the property of name
to hidden
. In order not to mess up with the original Common
which might require read-only
name by other objects, we create common2
instead.
01e4a80
to
7357a7c
Compare
fbc5a09
to
2303f95
Compare
2303f95
to
9b4229a
Compare
lib/install/management/delete.go
Outdated
continue | ||
} | ||
if err != nil { | ||
errs = append(errs, err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if error happens, we should continue to avoid delete vch by mistake
lib/migration/migrator_test.go
Outdated
@@ -34,10 +36,10 @@ func setUp() { | |||
// register sample plugin into test | |||
log.SetLevel(log.DebugLevel) | |||
trace.Logger.Level = log.DebugLevel | |||
version.MaxPluginVersion = 1 | |||
version.MaxPluginVersion = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version.MaxPluginVersion = version.MaxPluginVersion + 1
@@ -31,7 +31,7 @@ import ( | |||
// If only a couple of items changed in the configuration, you don't have to copy all VirtualContainerHost. Only define the few items used by | |||
// this upgrade plugin will simplify the extraconfig encoding/decoding process | |||
const ( | |||
version = 1 | |||
version = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emlin No need to do this. I will fix it.
lib/migration/plugins/init.go
Outdated
import ( | ||
_ "github.com/vmware/vic/pkg/version/plugin2" | ||
_ "github.com/vmware/vic/pkg/version/plugin3" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugin sounds really bad, IMHO this should be migrator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm.. migrator is good, but I have used that in another place. 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then lets rename all in a separate PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gonna need a new name for the original migrator as well
lib/portlayer/exec/container.go
Outdated
} | ||
|
||
fullName := fmt.Sprintf("%s-%s", prettyName, shortID) | ||
task, err := c.vm.VirtualMachine.Rename(ctx, fullName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using vm.WaitForResult method, that will retry for TaskInProgress error
lib/portlayer/exec/container.go
Outdated
@@ -35,6 +35,7 @@ import ( | |||
"github.com/vmware/vic/pkg/vsphere/vm" | |||
|
|||
log "github.com/Sirupsen/logrus" | |||
"github.com/docker/docker/pkg/stringid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have portlayer rely on docker package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I will remove this from portlayer.
lib/vicadmin/validate.go
Outdated
self2 := vm.NewVirtualMachineFromVM(ctx, sess, self) | ||
vchName, err = self2.Name(ctx) | ||
if err != nil { | ||
log.Infof("Unable to get VCH name: %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err != nil, vchname will be ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emlin No, it will be VCH
instead of "".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How? You have reset vchName at line 226
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be ""
. In any case you should return an error from this function if err != nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err != nil {
vchName = defaultVchName
...
}
I think it's fine not to return err since the original cod just attempts to get a name and if not present, then just set to "VCH"
Notes string `vic:"0.1" scope:"hidden" key:"notes"` | ||
} | ||
|
||
type NewVirtualContainerHostConfigSpec struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like the method name to create struct VirtualContainerHostConfigSpec
- update test cases with checking for datastore_type
- pass specconfig as argument to util.DisplayName() - panic when data migration fails
4ffb067
to
607b057
Compare
The Name field was changed to be hidden from the guest in vmware#4134 to support docker rename. This was a compromise needed due to an ESX vigor bug, see vmware#5533. This *requires* that users be on a version of ESX that has the fix or bad things may happen to cVM configurations. I have not tracked down what build versions that entails and have not code a check mechanism. This change has the effect of the hostname of the VCH endpointVM being set to the name of the VCH. It also allows additional live reconfiguration such as hotadd to a bridge network, in general modification of all container configuration live is possible, if not necessarily applied.
The Name field was changed to be hidden from the guest in vmware#4134 to support docker rename. This was a compromise needed due to an ESX vigor bug, see vmware#5533. This *requires* that users be on a version of ESX that has the fix or bad things may happen to cVM configurations. I have not tracked down what build versions that entails and have not code a check mechanism. This change has the effect of the hostname of the VCH endpointVM being set to the name of the VCH. It also allows additional live reconfiguration such as hotadd to a bridge network, in general modification of all container configuration live is possible, if not necessarily applied.
The Name field was changed to be hidden from the guest in vmware#4134 to support docker rename. This was a compromise needed due to an ESX vigor bug, see vmware#5533. This *requires* that users be on a version of ESX that has the fix or bad things may happen to cVM configurations. I have not tracked down what build versions that entails and have not code a check mechanism. This change has the effect of the hostname of the VCH endpointVM being set to the name of the VCH. It also allows additional live reconfiguration such as hotadd to a bridge network, in general modification of all container configuration live is possible, if not necessarily applied.
Fixes #3972, #4075
This PR is the first step for
docker rename
support.added
ExecutorConfigCommon
incontainer_vm.go
with the property of "name" setting to "hidden", so that the containerVM'scommon/name
could be persisted to the vmx file while the user performsdocker rename
on a running containerVM/etc/hosts
does not contain containerName anymore sincecommon/name
is set to "hidden". Therefore, we don't need to update/etc/hosts
while renaming a container. This will solve tether should update (instead of appending) containerName in /etc/hosts after changing the VM name in guestinfo #4075when creating the containerVM, set the VM display name to
containerName-containerShortID
and datastore folder name tocontainerID
for non-vsan setup, and set both names tocontainerName-containerShortID
for vsan setupUpdated all the relevant test cases where the containerVM's display name does not match with the new naming convention
added a test case in 1-04-Docker-Create.robot: After creating a container, check that the VM displayname=containerName-shortID and datatstore folder name=containerID
added a plugin for VCH upgrade since we changed the scope of
common.name
of the VCHadded a plugin for containerVM data migration since we changed the scope of
common.name
of containerVMs (relies on Migrate container configuration in portlayer in memory #4146 )in vicadmin, obtain the VCH name from vsphere instead of from within the VCH (since the VCH hostname is hidden)