Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Add to import image from local #33

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Add to import image from local #33

merged 1 commit into from
Sep 23, 2020

Conversation

hyoung-90
Copy link
Collaborator

@hyoung-90 hyoung-90 commented Aug 7, 2020 •
edited
Loading

User can import a local image using hostPath source. User should create qcow2 file of disk.img in specific path and create vmim using that host path.

VirtualMachineImageHostPath.yaml

apiVersion: hypercloud.tmaxanc.com/v1alpha1
kind: VirtualMachineImage
metadata:
  name: localvmim
spec:
  source:
    hostPath:
      path: /mnt/hy
      nodeName: young
  snapshotClassName: csi-rbdplugin-snapclass
  pvc:
    volumeMode: Block
    accessModes:
      - ReadWriteMany
    resources:
      requests:
        storage: "3Gi"
    storageClassName: rook-ceph-block

@woohhan
Copy link
Contributor

woohhan commented Aug 18, 2020 •
edited
Loading

Please edit dbox

@woohhan
Copy link
Contributor

woohhan commented Aug 18, 2020

If we have no matching nodeName, the pod is pending. Is this enough? or is it an error?

@woohhan
Copy link
Contributor

woohhan commented Aug 18, 2020 •
edited by hbinkim
Loading

The nodeName field is enough as node selector? or could a label be used in the future?

@hbinkim
Copy link
Contributor

hbinkim commented Aug 18, 2020 •
edited
Loading

User guide is needed to update including explanations how to import a local vm image file. This PR's description is also needed to update with using cat and kubectl with WriteBlockPath instead of stagingPath.

woohhan
woohhan reviewed Aug 18, 2020
View reviewed changes
deploy/crds/hypercloud.tmaxanc.com_v1alpha1_localuploadproxy_cr.yaml Outdated
metadata:
name: example-localuploadproxy
spec:
nodeName: young
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use more general name

@woohhan
Copy link
Contributor

woohhan commented Aug 18, 2020

the 'kubectl exec' command can run any kubernetes node. If so, do we need to specify the node?

hbinkim
hbinkim reviewed Aug 18, 2020
View reviewed changes
Copy link
Contributor

@hbinkim hbinkim left a comment

Choose a reason for hiding this comment

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

Need English translation for following lines:

kubevirt-image-service/pkg/controller/virtualmachineimage/virtualmachineimage_controller.go

Line 100 in c7eba64

// pvc가 없으면 상태를 업데이트하고 pvc를 생성한다.

kubevirt-image-service/pkg/controller/virtualmachineimage/virtualmachineimage_controller.go

Line 104 in c7eba64

// imported=false인 경우 스크래치 pvc를 생성한다.

kubevirt-image-service/pkg/controller/virtualmachineimage/virtualmachineimage_controller.go

Line 108 in c7eba64

// imported=false인 경우 임포터파드가 없으면 만든다. 있으면 컴플리트인지 확인해서 imported=true로 변경한다.

kubevirt-image-service/pkg/controller/virtualmachineimage/virtualmachineimage_controller.go

Line 112 in c7eba64

// imported=true인 경우 스냅샷이 없으면 만들고, readyToUse를 true로 변경한다.

@hyoung-90 hyoung-90 force-pushed the local branch 3 times, most recently from ded77a8 to 0252c5f Compare August 20, 2020 02:30
@hyoung-90
Copy link
Collaborator Author

hyoung-90 commented Aug 20, 2020 •
edited
Loading

Need English translation for following lines:

kubevirt-image-service/pkg/controller/virtualmachineimage/virtualmachineimage_controller.go

Line 100 in c7eba64

// pvc가 없으면 상태를 업데이트하고 pvc를 생성한다.

kubevirt-image-service/pkg/controller/virtualmachineimage/virtualmachineimage_controller.go

Line 104 in c7eba64

// imported=false인 경우 스크래치 pvc를 생성한다.

kubevirt-image-service/pkg/controller/virtualmachineimage/virtualmachineimage_controller.go

Line 108 in c7eba64

// imported=false인 경우 임포터파드가 없으면 만든다. 있으면 컴플리트인지 확인해서 imported=true로 변경한다.

kubevirt-image-service/pkg/controller/virtualmachineimage/virtualmachineimage_controller.go

Line 112 in c7eba64

// imported=true인 경우 스냅샷이 없으면 만들고, readyToUse를 true로 변경한다.

Since it is not related to this pull request, I will modify it in another pull request. #38

@hyoung-90 hyoung-90 force-pushed the local branch 3 times, most recently from b30eb8b to 7069e37 Compare August 20, 2020 06:48
@hyoung-90 hyoung-90 mentioned this pull request Aug 20, 2020
Open
@hyoung-90
Copy link
Collaborator Author

the 'kubectl exec' command can run any kubernetes node. If so, do we need to specify the node?

That's right. It works normally on any node, so we don't need to specify node name.

@hyoung-90
Copy link
Collaborator Author

Please edit dbox

Done

@hyoung-90
Copy link
Collaborator Author

User guide is needed to update including explanations how to import a local vm image file. This PR's description is also needed to update with using cat and kubectl with WriteBlockPath instead of stagingPath.

Done

@hyoung-90 hyoung-90 requested review from hbinkim and woohhan August 21, 2020 01:29
@hyoung-90 hyoung-90 force-pushed the local branch 2 times, most recently from a901fca to ca18dd3 Compare August 24, 2020 02:36
hbinkim
hbinkim reviewed Aug 24, 2020
View reviewed changes
Copy link
Contributor

@hbinkim hbinkim left a comment

Choose a reason for hiding this comment

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

Ah looks like we need to consider how to create VMV for local uploaded VMIM.

docs/USERGUIDE.md Outdated Show resolved Hide resolved
@hyoung-90 hyoung-90 force-pushed the local branch 3 times, most recently from f2114b2 to eed9bb8 Compare August 26, 2020 01:44
@hyoung-90 hyoung-90 force-pushed the local branch 2 times, most recently from 78776bc to 9cac1db Compare September 17, 2020 03:08
@hyoung-90 hyoung-90 changed the title [WIP] Add to import image from local Add to import image from local Sep 17, 2020
woohhan
woohhan reviewed Sep 17, 2020
View reviewed changes
dbox Outdated Show resolved Hide resolved
woohhan
woohhan reviewed Sep 17, 2020
View reviewed changes
pkg/controller/virtualmachineimage/virtualmachineimage_controller.go Outdated
} else if src == sourceTypeVolume {
// check node name is not empty when source is hostPath volume
if r.vmi.Spec.Source.Volume.HostPath != nil && r.vmi.Spec.NodeName == "" {
return goerrors.New("when the source is hostPath volume, the node name must not be empty")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we just return error, how to the user figure out the vmim is in error state? we can see the error logs but there's no error message in vmim.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

user can check the error state through kubectl get vmim and the error message through kubectl describe vmim.
$ kubectl get vmim

NAME        STATE
localvmim   Error

$ kubectl describe vmim

Status:
  Conditions:
    Last Transition Time:  2020-09-18T01:16:24Z
    Message:               no source set for vmim
    Reason:                SeeMessages
    Status:                False
    Type:                  ReadyToUse
  State:                   Error

Isn't that enough?

@hyoung-90 hyoung-90 force-pushed the local branch 5 times, most recently from 6195349 to 016d70e Compare September 21, 2020 08:43
@hyoung-90 hyoung-90 force-pushed the local branch 3 times, most recently from 9bc2e66 to 70a2e00 Compare September 22, 2020 05:47
@hbinkim
Copy link
Contributor

hbinkim commented Sep 22, 2020

LGTM

@woohhan
Copy link
Contributor

woohhan commented Sep 22, 2020

LGTM overall except a few issues

@hyoung-90 hyoung-90 force-pushed the local branch 2 times, most recently from f0ff9be to 0455141 Compare September 22, 2020 09:20
@hookak
Copy link
Contributor

hookak commented Sep 22, 2020

lgtm

@hyoung-90
Add to import image from local
3104ef2 
- add hostpath source to vmim
- user can use hostpath source to import local image
@hbinkim
Copy link
Contributor

hbinkim commented Sep 23, 2020

LGTM

@hyoung-90 hyoung-90 merged commit 6490e39 into master Sep 23, 2020
@hyoung-90 hyoung-90 deleted the local branch September 23, 2020 08:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@woohhan woohhan woohhan left review comments

@hbinkim hbinkim hbinkim left review comments

@hookak hookak Awaiting requested review from hookak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hyoung-90 @woohhan @hbinkim @hookak