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

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

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:

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

@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
…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
…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
6 tasks
mtrensch pushed a commit to mtrensch/iotedge that referenced this issue Nov 21, 2022
…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
…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
…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
…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
…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.

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

Successfully merging a pull request may close this issue.

2 participants