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.

Sign up for GitHub

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

Jump to bottom

iotedge check "container-engine-logrotate" should not require rotate options when driver is "local" #6754

Open
damonbarry opened this issue Nov 2, 2022 · 3 comments · May be fixed by #6793
Open

iotedge check "container-engine-logrotate" should not require rotate options when driver is "local" #6754

damonbarry opened this issue Nov 2, 2022 · 3 comments · May be fixed by #6793
Labels
customer-reported iotedge no-issue-activity

Comments

@damonbarry
Copy link
Member

Expected Behavior

iotedge check should not warn me that the "Container engine is not configured to rotate module logs..." when I use Docker's local log driver. The container engine is configured to rotate module logs; the local driver provides default values for max-size and max-file if the user does not explicitly give them.

Current Behavior

IoT Edge docs recommend that I use Docker's local log driver. When I follow that recommendation by creating daemon.json like this:

{
  "log-driver": "local"
}

...then the iotedge tool's "container-engine-logrotate" check emits a warning because I didn't explicitly add a log-opts object with max-file and max-size values to daemon.json.

Runtime Versions

  • aziot-edged [run iotedge version]: 1.4
  • Edge Agent [image tag (e.g. 1.0.0)]: 1.4
  • Edge Hub [image tag (e.g. 1.0.0)]: 1.4
  • Docker/Moby [run docker version]: any version that supports the local log driver

Additional Information

@drwill-ms originally filed this as doc issue MicrosoftDocs/azure-docs#100762. I'm filing this issue because I believe the fix should be made in iotedge check instead.
@damonbarry
Copy link
Member Author

I think we just need to add a match arm for "local" that behaves like the journald arm, here:

iotedge/edgelet/iotedge/src/check/checks/container_engine_logrotate.rs

Lines 58 to 62 in d796a36

match daemon_config.log_driver.as_deref() {
Some("journald") => return Ok(CheckResult::Ok),
None => return Ok(CheckResult::Warning(anyhow!(MESSAGE))),
_ => (),
}

@PatAltimore PatAltimore mentioned this issue Nov 3, 2022
Closed
@mtrensch
Copy link
Contributor

I've sent the PR for the journald part, but when looking deeper into it, it seems like all logging drivers, but the default json-file logger, do rotate logs automatically.

So maybe the following would be better instead of a separate match of "local" driver:

        match daemon_config.log_driver.as_deref() {
            None | Some("json-file") => {
                if let Some(log_opts) = &daemon_config.log_opts {
                    if log_opts.max_file.is_none() {
                        return Ok(CheckResult::Warning(anyhow!(MESSAGE)));
                    }

                    if log_opts.max_size.is_none() {
                        return Ok(CheckResult::Warning(anyhow!(MESSAGE)));
                    }
                } else {
                    return Ok(CheckResult::Warning(anyhow!(MESSAGE)));
                }
            },
            _ => (),
        }
        Ok(CheckResult::Ok)

mtrensch pushed a commit to mtrensch/iotedge that referenced this issue Nov 21, 2022
@MTrensch-hilscher
iotedge check: Only verify logging options for json-file driver (whic…
3336011 
…h is the default).

This fixes Azure#6754

NOTE: All other logging driver usually do logrotation on their own
mtrensch pushed a commit to mtrensch/iotedge that referenced this issue Nov 21, 2022
@MTrensch-hilscher
iotedge check: Only verify logging options for json-file driver (whic…
e5864e5 
…h is the default).

This fixes Azure#6754

NOTE: All other logging driver usually do logrotation on their own
@mtrensch mtrensch linked a pull request Nov 21, 2022 that will close this issue
Open
6 tasks
mtrensch pushed a commit to mtrensch/iotedge that referenced this issue Nov 21, 2022
@MTrensch-hilscher
iotedge check: Only verify logging options for json-file driver (whic…
993f724 
…h is the default).

This fixes Azure#6754

NOTE: All other logging driver usually do logrotation on their own
mtrensch added a commit to mtrensch/iotedge that referenced this issue Nov 21, 2022
@mtrensch
iotedge check: Only verify logging options for json-file driver (whic…
f0da82d 
…h is the default).

This fixes Azure#6754

NOTE: All other logging driver usually do logrotation on their own
mtrensch added a commit to mtrensch/iotedge that referenced this issue Nov 24, 2022
@mtrensch
iotedge check: Only verify logging options for json-file driver (whic…
23d02b0 
…h is the default).

This fixes Azure#6754

NOTE: All other logging driver usually do logrotation on their own
mtrensch added a commit to mtrensch/iotedge that referenced this issue Nov 30, 2022
@mtrensch
iotedge check: Only verify logging options for json-file driver (whic…
3f1c752 
…h is the default).

This fixes Azure#6754

NOTE: All other logging driver usually do logrotation on their own
mtrensch added a commit to mtrensch/iotedge that referenced this issue Nov 30, 2022
@mtrensch
iotedge check: Only verify logging options for json-file driver (whic…
2071f48 
…h is the default).

This fixes Azure#6754

NOTE: All other logging driver usually do logrotation on their own
@github-actions
Copy link

This issue is being marked as stale because it has been open for 30 days with no activity.

@mame-leadec mame-leadec mentioned this issue Sep 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
customer-reported iotedge no-issue-activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

iotedge check: Only verify logging options for json-file driver mtrensch/iotedge
2 participants
@mtrensch @damonbarry