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

Switch default logdriver and eventslogger to journald, if root #621

Merged
merged 1 commit into from
Aug 9, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jun 11, 2021

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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:

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

@rhatdan
Copy link
Member Author

rhatdan commented Jun 11, 2021

@giuseppe @vrothberg is there an easy way to see if journald will work for rootless users?

@rhatdan
Copy link
Member Author

rhatdan commented Jun 11, 2021

Attempting to fix/improve #580

@vrothberg
Copy link
Member

/hold

Can you open a test PR against Podman? We've had bad experiences in previous attempts. I am optimistic that it'll pass since we massaged the journald backends quite a bit but I want to make sure we merge after Podman CI is green.

@vrothberg
Copy link
Member

@giuseppe @vrothberg is there an easy way to see if journald will work for rootless users?

https://github.com/containers/podman/blob/master/pkg/systemd/dbus.go has some code for it. I think that if we can connect to DBUS, we can default to journald.

@rhatdan
Copy link
Member Author

rhatdan commented Jun 11, 2021

That will tell us we have a dbus connection, but not necessarily if we can write journald messages.

I will open a PR for Podman.

@rhatdan rhatdan changed the title Switch default logdriver and eventslogger to journald, if root [WIP] Switch default logdriver and eventslogger to journald, if root Jun 11, 2021
@rhatdan
Copy link
Member Author

rhatdan commented Jun 11, 2021

Currently this patch just uses file for rootless systems, even if the system supports rootless journald. Since some systems give permission denied if you try to write to the journal without being in the "wheel" group.

@vrothberg
Copy link
Member

Good point. There may be group access in between. Then something as below should work:

import "github.com/coreos/go-systemd/v22/sdjournal"

func CanAccessJournald() bool {
   journal, err := sdjournal.NewJournal()
   if err != nil {                       
           logrus.Debugf("Cannot access journald: %v", err)
           return false
   }                         
 return true            
}

@rhatdan
Copy link
Member Author

rhatdan commented Jun 11, 2021

containers/podman#10653

@vrothberg
Copy link
Member

Good point. There may be group access in between. Then something as below should work:

import "github.com/coreos/go-systemd/v22/sdjournal"

func CanAccessJournald() bool {
   journal, err := sdjournal.NewJournal()
   if err != nil {                       
           logrus.Debugf("Cannot access journald: %v", err)
           return false
   }                         
 return true            
}

I think we need to defer journal.Close() as well.

@rhatdan
Copy link
Member Author

rhatdan commented Jun 11, 2021

This adds a requirement for cgo...

@rhatdan
Copy link
Member Author

rhatdan commented Jun 11, 2021

 $ make
GOOS=linux GOARCH= go build -tags "" ./... # attempt to build without tags
GOOS=linux GOARCH=386 go build -tags "containers_image_openpgp,systemd" ./...
package github.com/containers/common/libimage
	imports github.com/containers/common/pkg/config
	imports github.com/coreos/go-systemd/v22/sdjournal
	imports github.com/coreos/go-systemd/v22/internal/dlopen: build constraints exclude all Go files in /home/dwalsh/go/src/github.com/containers/common/vendor/github.com/coreos/go-systemd/v22/internal/dlopen
make: *** [Makefile:41: build-cross] Error 1

@vrothberg
Copy link
Member

May need to add CGO_ENABLED ?= 1 to the Makefile. Does that resolve the build?

@rhatdan
Copy link
Member Author

rhatdan commented Jun 11, 2021

That is causing us to pull in lots of other libraries and files including -devmapper.

@rhatdan rhatdan force-pushed the events branch 4 times, most recently from 972bdb8 to 7d9e76e Compare June 24, 2021 18:58
return
}
journal, err := sdjournal.NewJournal()
if err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

@giuseppe @vrothberg @nalind Do you know if this is enough to check if we can use the journal?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the implementation will either manage to open the journal for reading and return a handle for it, or return an error. The APIs for sending data to the journal don't go through the handle, so I think we have to take it on faith that messages we send to the journal will be retrievable through the handle.

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM

This will fix hte eventslog and log files from growing huge,
Lets journald handling rolling logs.

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan rhatdan changed the title [WIP] Switch default logdriver and eventslogger to journald, if root Switch default logdriver and eventslogger to journald, if root Aug 9, 2021
@rhatdan
Copy link
Member Author

rhatdan commented Aug 9, 2021

@TomSweeneyRedHat @mheon PTAL
Have the PR for Podman working now, Would like to get this into RHEL9.

@containers/podman-maintainers PTAL

@jwhonce
Copy link
Member

jwhonce commented Aug 9, 2021

LGTM


return DefaultLogDriver

return "k8s-file"
Copy link
Member

Choose a reason for hiding this comment

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

Should you still set DefaultLogDriver to k8s-file, return it here, and then just system it to systems in the appropriate places?

Copy link
Member Author

Choose a reason for hiding this comment

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

No because we want DefaultLogDriver to be indicated as journald now.

@TomSweeneyRedHat
Copy link
Member

LGTM
one question over the default setting.

@rhatdan rhatdan added the lgtm label Aug 9, 2021
@openshift-ci openshift-ci bot merged commit a5b706b into containers:main Aug 9, 2021
@vrothberg
Copy link
Member

We need to flip back: containers/podman#10653 was not green and it's now causing c/common to break in Podman.

@vrothberg
Copy link
Member

We need to flip back: containers/podman#10653 was not green and it's now causing c/common to break in Podman.

Correction. We're good! @nalind is pushing the PR in Podman over the finish line in containers/podman#11263.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants