-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Use existing global var criSupportedLogDrivers #44126
Conversation
Hi @xiangpengzhao. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with 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. I understand the commands that are listed here. |
glog.Errorf("Cannot create symbolic link because container log file doesn't exist!") | ||
} else { | ||
glog.V(5).Infof("Unsupported logging driver: %s", dockerLoggingDriver) | ||
for _, driver := range criSupportedLogDrivers { |
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 logic is incorrect. You'll end up logging the error for each supported driver. How about just creating an isSupportedLogDriver(driver string) bool
helper function?
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.
Actually, there is already such a function, we should use it https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/dockershim/docker_service.go#L483-L494
glog.V(5).Infof("Unsupported logging driver: %s", dockerLoggingDriver) | ||
for _, driver := range criSupportedLogDrivers { | ||
if dockerLoggingDriver == driver { | ||
glog.Errorf("Cannot create symbolic link because container log file doesn't exist!") |
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.
shouldn't this return an error instead of logging 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.
@timstclair Thanks for your reviewing and comments. IMO, when we find realPath empty, we just need to give users some logs for debugging. We don't want to return error for any conditions when realPath is empty. Former discussion in #38691 with @Random-Liu and other folks.
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 remember the problem is that some InspectContainer
of fake docker client doesn't return the log path.
If we return error here, it will break many unit tests.
And it's fine to not return error here, because:
- In real case, real path should never be nil;
- The symlink is a best effort thing now. Even if we return an error here, next
SyncPod
will think the container is running, because the container is started already.
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.
Ok, in that case this should be Info or Warning level, not Error.
@@ -243,10 +239,12 @@ func (ds *dockerService) createContainerLogSymlink(containerID string) error { | |||
glog.V(10).Infof("Docker logging driver is %s", dockerLoggingDriver) | |||
} | |||
|
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.
(above, line 236) Shouldn't that glog.Errorf
call return an error instead? We don't want to carry on if the dockerInfo call didn't return anything useful...
Comment addressed. Used IsCRISupportedLogDriver and left logic as is. |
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.
LGTM with one nit.
} else { | ||
dockerLoggingDriver = dockerInfo.LoggingDriver | ||
glog.V(10).Infof("Docker logging driver is %s", dockerLoggingDriver) | ||
glog.Errorf("Failed to check supported logging driver by CRI: %v", 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.
nit: Would better to return here.
4bd037a
to
106a9e0
Compare
nit addressed. PTAL @Random-Liu |
/approve |
@timstclair Are you ok with this PR? |
Thanks @timstclair ! Inline comment addressed. PTAL. cc @Random-Liu |
…RISupportedLogDriver
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Random-Liu, timstclair, xiangpengzhao
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 45508, 44258, 44126, 45441, 45320) |
What this PR does / why we need it:
Use existing global var
criSupportedLogDrivers
defined in docker_service.go. If CRI supports other log drivers in the future, we will only need to modify that global var.cc @Random-Liu