Skip to content

Non-systemd udev path + adjusted unit test#6882

Closed
red15 wants to merge 2 commits intoinfluxdata:masterfrom
red15:master
Closed

Non-systemd udev path + adjusted unit test#6882
red15 wants to merge 2 commits intoinfluxdata:masterfrom
red15:master

Conversation

@red15
Copy link
Copy Markdown
Contributor

@red15 red15 commented Jan 9, 2020 •
edited by sjwang90
Loading

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Now only probing 2 paths at most.

This is a rewrite with lessons learned by writing #5180 .
Closes #3900

@red15 red15 requested a review from danielnelson January 17, 2020 09:24
@sjwang90 sjwang90 added area/system fix pr to fix corresponding bug labels Nov 24, 2020
srebhan
srebhan approved these changes Dec 4, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

While I would rather like to see the initialization of ic.udevDataPath in a Init() function, I can live with what you have now.

@srebhan srebhan removed the request for review from danielnelson December 4, 2020 10:09
@srebhan srebhan self-assigned this Dec 4, 2020
@srebhan srebhan added ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. and removed ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Dec 4, 2020
@srebhan
Copy link
Copy Markdown
Member

srebhan commented Dec 4, 2020

@red15: One thing, can you please rebase to run the CI checks again!? Thanks!

@red15
Copy link
Copy Markdown
Contributor Author

red15 commented Dec 5, 2020

While I would rather like to see the initialization of ic.udevDataPath in a Init() function, I can live with what you have now.

Didn't know there was the ability have Init() function in a module like this, I'm not very familiar with the project as a whole.

One thing, can you please rebase to run the CI checks again!? Thanks!

Done, although now the macOS CI is failing for some reason ?

@srebhan
Copy link
Copy Markdown
Member

srebhan commented Dec 7, 2020

Yeah, the example missing it but basically you can use

func (s *DiskIO) Init() error {
    ...
    return nil
}

The function is called after the config has been initialized, so it is a nice way to do the one-time things at construction...

Do you see a chance to address the Init() comment?

@red15
Copy link
Copy Markdown
Contributor Author

red15 commented Dec 7, 2020

Do you see a chance to address the Init() comment?

Unlikely to happen soon, I'll need to setup my build environment again and I'm worried that the test scenario will make this more complicated as I'd have to make sure the test mocks are surviving the Init() phase.

@srebhan
Copy link
Copy Markdown
Member

srebhan commented Dec 14, 2020

I think we can live with the missing Init() and fix it later. Patches welcome!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Dec 14, 2020
@helenosheaa
Copy link
Copy Markdown
Member

helenosheaa commented Jan 27, 2021

@red15 can you please rebase this against master and temove telegraf from your CircleCI account as it's not running the mac test

@Hipska Hipska added the plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins label Jan 29, 2021
red15 added 2 commits January 29, 2021 19:28
@red15
Alter unit test method to use DiskInfoCache.udevDataPath
897b3a3 
Removing the udevPath global variable and it's dependencies.
This sets us up to allow better detection of udevDataPath to support
non-systemd linux versions.
@red15
Support older non-systemd udev data paths
ee30fe1 
Should properly solve (with minimal file probes) the udev mounting
problem.
@red15
Copy link
Copy Markdown
Contributor Author

red15 commented Jan 29, 2021

@red15 can you please rebase this against master and temove telegraf from your CircleCI account as it's not running the mac test

Rebased on the master, I removed the CircleCI project on my side.

srebhan
srebhan reviewed Feb 1, 2021
View reviewed changes
@srebhan
Copy link
Copy Markdown
Member

srebhan commented Feb 1, 2021

@helenosheaa any idea why the macdeps test is not executed?

@helenosheaa
Copy link
Copy Markdown
Member

helenosheaa commented Feb 1, 2021

@srebhan it's connected to his own telegraf CircleCi project so it's not executing.

@red15 it still seems to be linking to your CircleCi rather than ours, any ideas?

@red15
Copy link
Copy Markdown
Contributor Author

red15 commented Feb 1, 2021

@red15 it still seems to be linking to your CircleCi rather than ours, any ideas?

This is because it's on my fork, (i'm owner) perhaps it would be easier to create a branch on the actual influxdata company/organization repository (i'm not able to create branches there), merge my change into that one then merge it to the influxdata/telegraf master branch.

@ssoroka
Copy link
Copy Markdown
Contributor

ssoroka commented Feb 1, 2021

@red15 it being on a fork shouldn't matter as a lot of people do that. Is it possible CircleCI created a Telegraf project for you and you need to delete it?

@red15
Copy link
Copy Markdown
Contributor Author

red15 commented Feb 1, 2021

As far as I can see I did already cancel the builds 3days ago, this is the last check worfklow that ran:
https://app.circleci.com/pipelines/github/red15/telegraf/328/workflows/740788ad-595e-49e1-8522-5d0c2ca7ad5e

I can't really see how to delete the telegraf project attached to my CircleCI account? If i look at my dashboard there are no followed projects.

@red15 red15 mentioned this pull request Feb 2, 2021
Merged
2 tasks
@red15 red15 closed this Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srebhan srebhan srebhan approved these changes

Assignees

@srebhan srebhan

Labels

area/system fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

enhancement - inputs/system/diskio_linux - configurable udev data path

6 participants

@red15 @srebhan @helenosheaa @ssoroka @Hipska @sjwang90