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

resourceread: use lib function for loading machine configs #195

Merged
merged 1 commit into from Dec 5, 2018

Conversation

Projects
None yet
6 participants
@darkmuggle
Contributor

darkmuggle commented Nov 28, 2018

Use lib/resourceread for loading machineconfigs for the daemon. Added
basic testing for load function.

@ashcrow

Initial review this looks really good. Thank you for adding tests! Will merge if CI comes back happy.

@@ -15,14 +15,13 @@ import (
ignv2_2types "github.com/coreos/ignition/config/v2_2/types"
"github.com/golang/glog"
drain "github.com/openshift/kubernetes-drain"
resr "github.com/openshift/machine-config-operator/lib/resourceread"

This comment has been minimized.

@ashcrow

ashcrow Nov 28, 2018

Member

Note: the rest of the code base uses resourceread without an alias, but I'm still OK with this.

This comment has been minimized.

@darkmuggle

darkmuggle Nov 28, 2018

Contributor

Fixed

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 28, 2018

/ok-to-test

// ReadMachineConfigV1 reads raw MachineConfig object from bytes. Returns MachineConfig and error.
func ReadMachineConfigV1(objBytes []byte) (*mcfgv1.MachineConfig, error) {
mc, err := runtime.Decode(mcfgCodecs.UniversalDecoder(mcfgv1.SchemeGroupVersion), objBytes)
if err == nil {

This comment has been minimized.

@abhinavdahiya

abhinavdahiya Nov 28, 2018

Member
if err !=nil {
    return nil, err
}
mc, ok := obj.(*mcfgv1.MachineConfig)
if !ok {
    return nil, <error describing this>
}
return mc, nil

This comment has been minimized.

@darkmuggle

darkmuggle Nov 28, 2018

Contributor

Thanks for the feedback...I've hardened the logic and repushed.

@abhinavdahiya

If you are returning error please don't panic. return errors

@darkmuggle darkmuggle force-pushed the darkmuggle:readapply branch from 14d7741 to 4bd4aaf Nov 28, 2018

@darkmuggle

This comment has been minimized.

Contributor

darkmuggle commented Nov 29, 2018

@abhinavdahiya the rationale for ReadMachineConfigV1 was create a check function that could be used in other parts of the code. Before this commit, individual callers that wanted to parse a MachineConfig had to implement their own logic [1] . This change removes [1] in favor of ReadMachineConfigV1, Since ReadMachineConfigV1OrDie is used (when a panic is desirable), ReadMachineConfigV1 handles the parsing of the config for the rest of the code.

[1]

mcfgScheme := runtime.NewScheme()
mcfgv1.AddToScheme(mcfgScheme)
mcfgCodecs := serializer.NewCodecFactory(mcfgScheme)
requiredObj, err := runtime.Decode(mcfgCodecs.UniversalDecoder(mcfgv1.SchemeGroupVersion), content)
if err == nil {
glog.V(2).Infof("onceFrom file is of type MachineConfig")
mcConfig := requiredObj.(*mcfgv1.MachineConfig)
return mcConfig, MachineConfigMCFileType, contentFrom, nil
}

mc, ok := m.(*mcfgv1.MachineConfig)
if !ok {
return nil, errDecode

This comment has been minimized.

@abhinavdahiya

abhinavdahiya Nov 29, 2018

Member

runtime.Decode(mcfgCodecs.UniversalDecoder(mcfgv1.SchemeGroupVersion), objBytes) will parse raw bytes of any of the types in

func addKnownTypes(scheme *runtime.Scheme) error {
scheme.AddKnownTypes(SchemeGroupVersion,
&MCOConfig{},
&ControllerConfig{},
&ControllerConfigList{},
&MachineConfig{},
&MachineConfigList{},
&MachineConfigPool{},
&MachineConfigPoolList{},
)

So this m.(*mcfgv1.MachineConfig) is that the raw byte was actually MachineConfig object. So the error should be fmt.Errorf("expected *mcfvgv1.MachineConfig bute found %T", m)

This comment has been minimized.

@darkmuggle

darkmuggle Nov 29, 2018

Contributor

Ah, indeed 👍 I've made the error more descriptive and updated the other others too.

@darkmuggle darkmuggle force-pushed the darkmuggle:readapply branch from 4bd4aaf to 124181c Nov 29, 2018

@abhinavdahiya

This comment has been minimized.

Member

abhinavdahiya commented Nov 29, 2018

/approve

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 29, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 29, 2018

/test e2e-aws

More flakes:

net/http: TLS handshake timeout
@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 29, 2018

/test e2e-aws

🙄

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 29, 2018

/test e2e-aws

8 similar comments
@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 30, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 30, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 30, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 30, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 30, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 30, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 30, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Nov 30, 2018

/test e2e-aws

@openshift-bot

This comment has been minimized.

openshift-bot commented Dec 3, 2018

@darkmuggle: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@darkmuggle

This comment has been minimized.

Contributor

darkmuggle commented Dec 3, 2018

/test e2e-aws

@darkmuggle

This comment has been minimized.

Contributor

darkmuggle commented Dec 3, 2018

/test e2e-aws

2 similar comments
@ashcrow

This comment has been minimized.

Member

ashcrow commented Dec 4, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Dec 4, 2018

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Dec 4, 2018

could not wait for build: the build machine-config-controller failed after 5m13s with reason DockerBuildFailed: Docker build strategy has failed.

Failure to build

/test e2e-aws

@ashcrow

This comment has been minimized.

Member

ashcrow commented Dec 4, 2018

/retest

@ashcrow

This comment has been minimized.

Member

ashcrow commented Dec 4, 2018

Looks like build issues related to dangling imports.

pkg/daemon/daemon.go:25:2: imported and not used: "github.com/openshift/machine-config-operator/vendor/k8s.io/apimachinery/pkg/runtime"
pkg/daemon/daemon.go:26:2: imported and not used: "github.com/openshift/machine-config-operator/vendor/k8s.io/apimachinery/pkg/runtime/serializer"
# github.com/openshift/machine-config-operator/pkg/daemon
pkg/daemon/daemon.go:25:2: imported and not used: "github.com/openshift/machine-config-operator/vendor/k8s.io/apimachinery/pkg/runtime"
pkg/daemon/daemon.go:26:2: imported and not used: "github.com/openshift/machine-config-operator/vendor/k8s.io/apimachinery/pkg/runtime/serializer"
@ashcrow

This comment has been minimized.

Member

ashcrow commented Dec 5, 2018

/retest

@ashcrow

This comment has been minimized.

Member

ashcrow commented Dec 5, 2018

@darkmuggle PTAL at the dangling imports.

resourceread: use lib function for loading machine configs
Use lib/resourceread for loading machineconfigs for the daemon. Added
basic testing for load function.

@darkmuggle darkmuggle force-pushed the darkmuggle:readapply branch from 4915cec to 7e1d426 Dec 5, 2018

@darkmuggle

This comment has been minimized.

Contributor

darkmuggle commented Dec 5, 2018

@ashcrow looks like merge conflicts were the root cause of the dangling imports. I've rebased from master, and pushed. Maybe it'll work now....

@darkmuggle

This comment has been minimized.

Contributor

darkmuggle commented Dec 5, 2018

@abhinavdahiya I believe your requested changes have been addressed? If so, would you mind removing your objection?

@abhinavdahiya

This comment has been minimized.

Member

abhinavdahiya commented Dec 5, 2018

@abhinavdahiya I believe your requested changes have been addressed? If so, would you mind removing your objection?

Oh, i forgot 😇

/approve

@ashcrow

This comment has been minimized.

Member

ashcrow commented Dec 5, 2018

/lgtm

@openshift-ci-robot

This comment has been minimized.

openshift-ci-robot commented Dec 5, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, ashcrow, darkmuggle

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,ashcrow]

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

@openshift-merge-robot openshift-merge-robot merged commit d9b254f into openshift:master Dec 5, 2018

5 checks passed

ci/prow/e2e-aws Job succeeded.
Details
ci/prow/images Job succeeded.
Details
ci/prow/rhel-images Job succeeded.
Details
ci/prow/unit Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment