-
Notifications
You must be signed in to change notification settings - Fork 231
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
Refactor to set pullsecret and clusterID #869
Conversation
pkg/crc/machine/machine.go
Outdated
rerr = m.ToError() | ||
}() | ||
|
||
// Update the user pull secret before kubelet start. |
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.
Looks like outdated comment? The kubelet was started just before.
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.
@cfergeau By default the pull secret set to null string in our bundle so logically we are updating the pull secret but since on UX side if we say updating the pull secret then user might think that there is already existing pull secret so we just say Adding user's pull secret
:)
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.
My problem with this comment is before kubelet start
since the code is:
if _, err := sd.Start("kubelet"); err != nil {
return fmt.Errorf("Error starting kubelet: %s", err)
}
//...
// Update the user pull secret before kubelet start.
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.
@cfergeau Ah I got it now, I will remove the before kublet start
part from comment.
One thing I forgot to mention, if we start to rely more on the host->VM DNS being functional, then it's a good opportunity to fix #854 to ensure it's there before we try to use it. |
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.
Since I can't easily comment commit by commit... first commit (dns checks) looks good to me.
Second commit, in addition to the comments I made, I believe the commit log could be extended to explain the change that is being made (instead of running commands on the cluster, we now run oc from the host to talk to the cluster).
This refactor allow us to perform ...
-> This refactor will allow us to perform..
as the changes which are mentioned are not done in this commit.
pkg/crc/network/utils.go
Outdated
return err | ||
} | ||
logging.Debugf("api.crc.testing resolved to %s", ips) | ||
ips, err = net.LookupIP(fmt.Sprintf("foo.%s", crcbundleMetadata.ClusterInfo.AppsDomain)) |
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.
We could be evil and use a randomly generated string to make sure people don't add foo.crc.testing to their /etc/hosts ;) (kidding)
pkg/crc/network/utils.go
Outdated
@@ -80,3 +82,17 @@ func DetermineHostIP(instanceIP string) (string, error) { | |||
|
|||
return "", errors.New("unknown error occurred") | |||
} | |||
|
|||
func CheckCRCLocalDNSReachableFromHost(crcbundleMetadata *bundle.CrcBundleInfo) error { | |||
ips, err := net.LookupIP(fmt.Sprintf("api.%s.%s", crcbundleMetadata.ClusterInfo.ClusterName, crcbundleMetadata.ClusterInfo.BaseDomain)) |
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.
Does this need to be retried for a few seconds in case the dnsmasq instance takes a bit of time to come up, or is this all synchronous?
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.
@cfergeau since we first check already on the VM which make sure that dnsmasq is running on the VM so on the host it should be good without retry.
pkg/crc/cluster/cluster.go
Outdated
var ( | ||
// This command make sure we stop the kubelet and clean up the pods | ||
// We also providing a 2 sec sleep so that stop pods get settled and | ||
// ready for remove. Without this 2 sec time sometime it happens some 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.
for removal
pkg/crc/cluster/cluster.go
Outdated
|
||
var ( | ||
// This command make sure we stop the kubelet and clean up the pods | ||
// We also providing a 2 sec sleep so that stop pods get settled and |
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.
'stopped pods' ?
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.
and 2 seconds rather than 2 sec, however I don't see a 2 seconds sleep in these crictl commands?
edit: this is managed in the function making use of this variable. I would make this a local variable to that function, this way it's easier to see what the 2 seconds refer to.
pkg/crc/cluster/cluster.go
Outdated
cmdArgs := []string{"patch", "secret", "pull-secret", "-p", | ||
fmt.Sprintf(`{"data":{".dockerconfigjson":"%s"}}`, base64OfPullSec), | ||
"-n", "openshift-config", "--type", "merge"} | ||
logging.Debugf("Executing oc %s\n", strings.Join(cmdArgs, " ")) |
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 belongs to oc.RunOcCommand rather than here if you want such a log.
logging.Debugf("Patching cluster config for new pull secret")
for a debug log which goes in this method.
This is also leaking the pull secret to debug logs.
pkg/crc/cluster/cluster.go
Outdated
} | ||
return nil | ||
} | ||
return errors.RetryAfter(80, addPullSecret, time.Second) |
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'd prefer if we did not have this somehow arbitrary RetryAfter in these methods. A prerequisite for calling this methods is that the api server is up, if needed we can add a method waiting for the api server to be up, and which would contain such a RetryAfter call.
pkg/crc/network/utils.go
Outdated
if err != nil { | ||
return err | ||
} | ||
logging.Debugf("api.crc.testing resolved to %s", ip) |
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.
logging.Debugf("api.%s resolved to %s", domainName, ip)
pkg/crc/network/utils.go
Outdated
if err != nil { | ||
return err | ||
} | ||
logging.Debugf("foo.apps-crc.testing resolved to %s", ip) |
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.
logging.Debugf("foo.%s resolved to %s", appsDomainName, ip)
(or somethnig similar :)
pkg/crc/network/utils.go
Outdated
@@ -92,3 +94,17 @@ func UriStringForDisplay(uri string) (string, error) { | |||
} | |||
return uri, nil | |||
} | |||
|
|||
func CheckCRCLocalDNSReachableFromHost(crcbundleMetadata *bundle.CrcBundleInfo) 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.
Wondering if we need the dependency on the bundle
package, or if this should be
func CheckCRCLocalDNSReachableFromHost(domainName, appsDomainName string) 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.
Still a couple of minor comments, but overall looks good to me :)
pkg/crc/machine/machine.go
Outdated
@@ -682,3 +678,32 @@ func updateSSHKeyPair(sshRunner *crcssh.SSHRunner) error { | |||
|
|||
return err | |||
} | |||
|
|||
func configureCluster(ocConfig oc.OcConfig, sshRunner *crcssh.SSHRunner, pullSecret string) (rerr error) { | |||
m := errors.MultiError{} |
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 can be moved in the defer lambda, as it's only used there.
pkg/crc/machine/machine.go
Outdated
if err := cluster.StopAndRemovePodsInVM(sshRunner); err != nil { | ||
m.Collect(err) | ||
} | ||
rerr = m.ToError() |
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.
wondering if we really need a MultiError here though .If sd.Stop() fails, it's very llikely cluster.StopAndRemovePodsInVM will fail too, so we could just report the sd.Stop() error in that case? And if sd.Stop succeeds, then we only have the StopAndRemovePodsInVM error to potentially report.
This check will make sure that host can able to resolve the dns for api and app domain of crc using dnsmasq which is running inside the VM.
This refactor will allow us to perform more cluster resource changes like add proxy, insecure registry etc. at cluster level. Now we are running oc commands directly from the host instead running those from the VM.
This refactor allow us to perform more cluster resource changes like
add proxy, insecure registry etc. at cluster level.
Note: Now we are going to depend on the dns even before approving the csr so if local dns is not working then CRC not even able to start.