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

asset/machines: update API version of MAO objects #1249

Conversation

abhinavdahiya
Copy link
Contributor

/cc @enxebre

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 13, 2019
@wking
Copy link
Member

wking commented Feb 14, 2019

This will need a vendor bump too, right?

@abhinavdahiya
Copy link
Contributor Author

This will need a vendor bump too, right?

Hmm, why?

@wking
Copy link
Member

wking commented Feb 14, 2019

To pick up openshift/cluster-api-provider-libvirt#113 for these consumers:

$ git grep 'libvirt.*v1alpha1' pkg
pkg/asset/cluster/tfvars.go:	libvirtprovider "github.com/openshift/cluster-api-provider-libvirt/pkg/apis/libvirtproviderconfig/v1alpha1"
pkg/asset/machines/libvirt/machines.go:	libvirtprovider "github.com/openshift/cluster-api-provider-libvirt/pkg/apis/libvirtproviderconfig/v1alpha1"
pkg/asset/machines/libvirt/machines.go:			APIVersion: "libvirtproviderconfig.k8s.io/v1alpha1",
pkg/asset/machines/master.go:	libvirtprovider "github.com/openshift/cluster-api-provider-libvirt/pkg/apis/libvirtproviderconfig/v1alpha1"
pkg/tfvars/libvirt/libvirt.go:	"github.com/openshift/cluster-api-provider-libvirt/pkg/apis/libvirtproviderconfig/v1alpha1"

@abhinavdahiya
Copy link
Contributor Author

But this PR is not changing provider apiversion. It is changing the machine object apiversion...

@wking
Copy link
Member

wking commented Feb 14, 2019

So is it catching us up with openshift/cluster-api-provider-libvirt#111? Seems like we'd want to completely transition to v1beta1, openshift.io APIs for libvirt, instead of tricking in one resource-type at a time. But I don't understand how all of this is wired up, so maybe transitioning machines without transitioning providers is fine.

@enxebre
Copy link
Member

enxebre commented Feb 14, 2019

Seems there's a left over from moving the machines on libvirt into machine.openshift.io/v1beta1 group which this PR fixes.
Separately for moving the libvirtProvider into machine.openshift.io we'll need to bump to openshift/cluster-api-provider-libvirt#113 and update

APIVersion: "libvirtproviderconfig.k8s.io/v1alpha1",

cc @ingvagabund

@cgwalters
Copy link
Member

$ oc  get --all-namespaces machinesets.machine.openshift.io
NAMESPACE               NAME              DESIRED   CURRENT   READY   AVAILABLE   AGE
openshift-machine-api   osiris-worker-0   1         1                             3m6s

🎆

However:

$ oc -n openshift-machine-api logs -c machine-controller pods/clusterapi-manager-controllers-56bb58b7bc-6t5p5                                                                                                     
I0215 17:46:26.810425       1 main.go:47] Registering Components.                                                                                                                                                 
I0215 17:46:26.811865       1 main.go:64] Starting the Cmd.                                                                                                                                                       
I0215 17:46:26.812271       1 reflector.go:202] Starting reflector *v1beta1.Machine (10h0m0s) from github.com/openshift/cluster-api-provider-libvirt/vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/infor
mers_map.go:126                                                                                                                                                                                                   
I0215 17:46:26.812373       1 reflector.go:240] Listing and watching *v1beta1.Machine from github.com/openshift/cluster-api-provider-libvirt/vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/informers_map
.go:126                                                                                                                                                                                                           
I0215 17:46:27.012477       1 controller.go:114] Running reconcile Machine for osiris-master-0
I0215 17:46:27.013271       1 reflector.go:202] Starting reflector *v1beta1.Cluster (10h0m0s) from github.com/openshift/cluster-api-provider-libvirt/vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/infor
mers_map.go:196
I0215 17:46:27.013371       1 reflector.go:240] Listing and watching *v1beta1.Cluster from github.com/openshift/cluster-api-provider-libvirt/vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/informers_map
.go:196
E0215 17:46:27.113523       1 runtime.go:66] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
/go/src/github.com/openshift/cluster-api-provider-libvirt/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:72
/go/src/github.com/openshift/cluster-api-provider-libvirt/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:65
/go/src/github.com/openshift/cluster-api-provider-libvirt/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:51
/usr/local/go/src/runtime/asm_amd64.s:573
/usr/local/go/src/runtime/panic.go:502
/usr/local/go/src/runtime/panic.go:63
/usr/local/go/src/runtime/signal_unix.go:388
/go/src/github.com/openshift/cluster-api-provider-libvirt/pkg/cloud/libvirt/actuators/machine/actuator.go:206
/go/src/github.com/openshift/cluster-api-provider-libvirt/vendor/github.com/openshift/cluster-api/pkg/controller/machine/controller.go:167
/go/src/github.com/openshift/cluster-api-provider-libvirt/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:207
/go/src/github.com/openshift/cluster-api-provider-libvirt/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:157
/go/src/github.com/openshift/cluster-api-provider-libvirt/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133
/go/src/github.com/openshift/cluster-api-provider-libvirt/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:134
/go/src/github.com/openshift/cluster-api-provider-libvirt/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88
/usr/local/go/src/runtime/asm_amd64.s:2361
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xf9c80b]

@ingvagabund
Copy link
Member

ingvagabund commented Feb 15, 2019

Since:

        // Cluster might be nil as some providers might not require a cluster object
        // for machine management.
        cluster, err := r.getCluster(ctx, m)
        if err != nil {
                // Just log the error here.
                klog.V(4).Infof("Cluster not found, machine actuation might fail: %v", err)
        }

Looks like we bumped cluster-api too much.
@cgwalters cluster object is missing. If you create one (even empty), the panic will disappear.

@cgwalters
Copy link
Member

Just gave this a try and:

$ oc get nodes
NAME                    STATUS   ROLES    AGE     VERSION
osiris-master-0         Ready    master   12m     v1.12.4+ec459b84aa
osiris-worker-0-csxkk   Ready    worker   4m54s   v1.12.4+ec459b84aa

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2019
@cgwalters
Copy link
Member

Though I'm also getting:

$ oc describe clusteroperator/monitoring
Name:         monitoring
Namespace:    
Labels:       <none>
Annotations:  <none>
API Version:  config.openshift.io/v1
Kind:         ClusterOperator
Metadata:
  Creation Timestamp:  2019-02-18T21:20:05Z
  Generation:          1
  Resource Version:    12062
  Self Link:           /apis/config.openshift.io/v1/clusteroperators/monitoring
  UID:                 f61b8e52-33c2-11e9-aba4-664f163f5f0f
Spec:
Status:
  Conditions:
    Last Transition Time:  2019-02-18T21:21:27Z
    Status:                False
    Type:                  Available
    Last Transition Time:  2019-02-18T21:22:59Z
    Message:               Rolling out the stack.
    Status:                True
    Type:                  Progressing
    Last Transition Time:  2019-02-18T21:21:27Z
    Message:               Failed to rollout the stack. Error: running task Updating Prometheus Operator failed: reconciling Prometheus Operator Service failed: updating Service object failed: services "prometheus-operator" is forbidden: caches not synchronized
    Status:                True
    Type:                  Failing
  Extension:               <nil>
  Related Objects:         <nil>
  Version:                 
Events:                    <none

But eh, at least I have workers again.

@cgwalters
Copy link
Member

/test e2e-aws

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@ingvagabund
Copy link
Member

/test e2e-aws

1 similar comment
@ingvagabund
Copy link
Member

/test e2e-aws

@ingvagabund
Copy link
Member

/retest

These changes should have happened with ecabe6f.
@crawford crawford force-pushed the fix-libvirt-machine-apiversion branch from ce0c0d5 to 37b25ea Compare February 20, 2019 01:01
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 20, 2019
@crawford
Copy link
Contributor

@abhinavdahiya I updated this based on our local testing (and added a commit message...)

@crawford crawford changed the title machines/libvirt: update apiversion to machine.openshift.io/v1beta1 asset/machines: update API version of MAO objects Feb 20, 2019
@abhinavdahiya
Copy link
Contributor Author

LGTM

@crawford
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, cgwalters, crawford

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,crawford]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@crawford
Copy link
Contributor

Failing tests:

The bootstrap user should successfully login with password decoded from kubeadmin secret [Suite:openshift/conformance/parallel]

Maybe we retested to quickly. That should have been marked as a flake. We'll deleted the CI ImageStreams and try again.

/retest

@openshift-merge-robot openshift-merge-robot merged commit 87ede7c into openshift:master Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants