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

#6811 Run conan config install automagically #6824

Merged
merged 19 commits into from Apr 29, 2020

Conversation

uilianries
Copy link
Member

@uilianries uilianries commented Apr 8, 2020

Hi!

The conan.config file now supports the new attribute config_install_interval, which accepts portions of time (e.g. 1m, 2h, 5d). It can not be configured by the environment variable.

When activated, Conan will generate the file sched.txt in the cache folder, which contains the last time when config install was executed.

If config install is executed as main command, it must be executed, and the time in sched.txt will be updated.

Changelog: Feature: Execute periodic config install command.
Docs: conan-io/docs#1679

closes #6811

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
conans/client/conf/__init__.py Outdated Show resolved Hide resolved
uilianries added 2 commits Apr 8, 2020
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries uilianries marked this pull request as ready for review Apr 8, 2020
@uilianries
Copy link
Member Author

@uilianries uilianries commented Apr 8, 2020

@memsharded please take a look on this idea, if it follows #6811

sched += timedelta(days=float(value))
return sched
except ValueError:
output.warn("The sched file is corrupted and will be removed.")
Copy link
Member Author

@uilianries uilianries Apr 8, 2020

Choose a reason for hiding this comment

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

Another option: blow up and let's the user solve the situation.

conans/client/command.py Outdated Show resolved Hide resolved
conans/client/conf/__init__.py Outdated Show resolved Hide resolved
conans/client/conf/config_installer.py Outdated Show resolved Hide resolved
uilianries added 3 commits Apr 8, 2020
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

@uilianries uilianries commented Apr 9, 2020

@memsharded Now it looks better I think.

conans/client/cache/cache.py Outdated Show resolved Hide resolved
conans/client/conf/config_installer.py Outdated Show resolved Hide resolved
conans/client/conf/config_installer.py Outdated Show resolved Hide resolved
conans/client/conf/config_installer.py Outdated Show resolved Hide resolved
conans/client/conf/config_installer.py Outdated Show resolved Hide resolved
conans/client/command.py Outdated Show resolved Hide resolved
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries uilianries changed the title #6811 Initial test for config install #6811 Run conan config install automagically Apr 9, 2020
Copy link
Member

@memsharded memsharded left a comment

Please try to simplify, something a bit more linear, I think it is possible.

pass


def is_config_install_scheduled(api):
Copy link
Member

@memsharded memsharded Apr 10, 2020

Choose a reason for hiding this comment

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

All this logic seems a bit more complicated than necessary. Maybe try relying on the modified time with the following?:

  • Let the sched file be empty, and use the file modified time
  • Get the last modified time of the sched file with os.path.getmtime("sched")
  • Do a os.utime("sched") to touch it and modified the time. (touch can create the file too)

:return: True, if it should occur now. Otherwise, False.
"""
try:
api.create_app()
Copy link
Member

@memsharded memsharded Apr 10, 2020

Choose a reason for hiding this comment

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

I'd prefer to access only the "cache" and the "config":

cache = ClientCache(self.cache_folder, self.out)
config = self.cache.config

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

@uilianries uilianries commented Apr 13, 2020

@memsharded Thanks for reviewing. Indeed it's much simpler now. Please, review again.

@@ -103,7 +103,8 @@ def decode_text(text, encoding="auto"):


def touch(fname, times=None):
os.utime(fname, times)
with open(fname, 'a'):
Copy link
Member Author

@uilianries uilianries Apr 13, 2020

Choose a reason for hiding this comment

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

small fix. os.utime requires the created file first.

Copy link
Member

@memsharded memsharded Apr 27, 2020

Choose a reason for hiding this comment

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

This might be wrong: https://stackoverflow.com/questions/42630281/utime-has-no-effect-in-windows

You need to close first the file handle, then utime it

@memsharded memsharded requested a review from jgsogo Apr 20, 2020
@memsharded memsharded added this to the 1.25 milestone Apr 20, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Someone told me once that "some people use the noatime mount flag on Linux for performance reasons"... will it affect this solution?

Also, if there is a .sched file in the repository we are installing, how will it behave? will it create an infinite loop? (the solution is not to add one more if to the config_installer::_process_folder function).


(if the previous statements are true, then:)

IMO logic is often easier if we store the timestamp for the next execution in the file, then we only need to check if now() > stored_time (defaults to 0 if file exists); then run; and the strategy to update the timestamp is easy: after any conan config install update the timestamp to now().
With this strategy there is still something hard: to update the file when someone changes the update period, from my POV we should identify that conan config set call and update the timestamp to now()

@uilianries
Copy link
Member Author

@uilianries uilianries commented Apr 22, 2020

Also, if there is a .sched file in the repository we are installing, how will it behave? will it create an infinite loop?

For every new conan install executed, it will touch again the .sched file, thus, the changed time will be different.

IMO logic is often easier if we store the timestamp for the next execution in the file, then we only need to check

Indeed, the current implementation needs to calculate the next execution for each iteration. Your solution performs better, as we need to calculate only once and update the file. Of course, it will increase the complexity a little bit, because we will need to parse the file content for each iteration

@memsharded I think we could follow Javi's idea

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Apr 23, 2020

Also, if there is a .sched file in the repository we are installing, how will it behave? will it create an infinite loop?

For every new conan install executed, it will touch again the .sched file, thus, the changed time will be different.

Please, correct me if I'm wrong (I haven't tried it). This implementation touches the .sched file before running the conan install XXXX, if the configuration repo contains a .shed file it will overwrite the one touched and it will contain a file with an older timestamp.... so next time Conan will run automagically the conan install.... that would override the .sched file with one with and old timestamp.... and next time Conan will detect configuration is outdated and.... again and again.

⬆️ That one is a problem with this approach and with the one I'm suggesting... and it is really easy IMO that someone commits and uploads the .sched file when populating the repo with the configuration. (Agree not our problem, but if it is easy to handle it...)

@uilianries
Copy link
Member Author

@uilianries uilianries commented Apr 23, 2020

This implementation touches the .sched file before running the conan install XXXX

Do you mean conan config install ? If yes, so no. It touches as last step for Conan config command.

@uilianries
Copy link
Member Author

@uilianries uilianries commented Apr 27, 2020

I see a problem with this strategy:

* You store the next time planned, lets say: 5 days

* Now you change the conan.conf time to execute every 1h

* The system still doesn't fire until 5 days pass

I didn't thing about updating the scheduler, but it makes sense. We would need change the timestamp in the sched file, but as we don't a reference about the last execution, or when config was updated, it would increase the complexity adding a new timestamp in the file. In this case, I prefer keeping the original implementation.

@@ -103,7 +103,8 @@ def decode_text(text, encoding="auto"):


def touch(fname, times=None):
os.utime(fname, times)
with open(fname, 'a'):
Copy link
Member

@memsharded memsharded Apr 27, 2020

Choose a reason for hiding this comment

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

This might be wrong: https://stackoverflow.com/questions/42630281/utime-has-no-effect-in-windows

You need to close first the file handle, then utime it

return sched


def _get_config_install_interval(config):
Copy link
Member

@memsharded memsharded Apr 27, 2020

Choose a reason for hiding this comment

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

This should be better implemented in the config class and return None if not defined.

@@ -247,3 +279,35 @@ def configuration_install(app, uri, verify_ssl, config_type=None,
else:
configs = [(c if c != config else config) for c in configs]
_save_configs(configs_file, configs)
try:
Copy link
Member

@memsharded memsharded Apr 27, 2020

Choose a reason for hiding this comment

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

I have realized that we already have a file for the "config install". Lets use the time of that file as the last time. No need to create further files! (maybe just add one touch to the file in one case)

Copy link
Member Author

@uilianries uilianries Apr 27, 2020

Choose a reason for hiding this comment

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

which file do you mean, conan.conf?

uilianries added 2 commits Apr 27, 2020
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

@uilianries uilianries commented Apr 27, 2020

@memsharded please, review again

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Apr 28, 2020

IMO logic is often easier if we store the timestamp for the next execution in the file, then we only need to check if now() > stored_time (defaults to 0 if file exists); then run; and the strategy to update the timestamp is easy: after any conan config install update the timestamp to now().
With this strategy there is still something hard: to update the file when someone changes the update period, from my POV we should identify that conan config set call and update the timestamp to now()

I see a problem with this strategy:

  • You store the next time planned, lets say: 5 days
  • Now you change the conan.conf time to execute every 1h
  • The system still doesn't fire until 5 days pass

No problem at all, if you change the conan.conf time, you set the next execution time to 0. Easy as cake and fewer hits to the database, but here this is not a concern for us as this affects only one timestamp. 🤷

I'll review with the previous approach in mind.

@memsharded
Copy link
Member

@memsharded memsharded commented Apr 28, 2020

I'll review with the previous approach in mind.

I am proposing some simplifications to the code, you better wait a few minutes until finished.

Copy link
Member

@jgsogo jgsogo left a comment

In this scenario:

10:00am: $ conan config install myrepo1
10:00am: $ conan config set general.config_install_interval=1h
11:30am: $ conan search xxxx

will run automagically conan config install myrepo1, and store 11:30 as the last execution time 👍 I keep working

12:15am: $ conan config install other-repo
12:40am: $ conan search xxxxx

I would expect it to run with the search command at 12:40am (> 11:30am +1h), but it didn't because config_install.json has been modified at 12:15am.

IMO it is preferable to have a false positive (it wasn't needed to run, but we did) than a false negative (it was needed, but we didn't). Is it how it works? wdyt?

@@ -684,3 +688,22 @@ def get_log_level_by_name(level_name):
"notset": logging.NOTSET
}
return levels.get(str(level_name).lower())

@property
def config_install_interval(self):
Copy link
Member

@memsharded memsharded Apr 28, 2020

Choose a reason for hiding this comment

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

@uilianries I have moved the validation of the interval here. It doesn't make sense that an incorrect value escape from this, and has to be validated in different places of the codebase, possibly with duplication of code.

now = datetime.now(gettz())
return now > sched
else:
raise ConanException("config_install_interval defined, but no config_install file")
Copy link
Member

@memsharded memsharded Apr 28, 2020

Choose a reason for hiding this comment

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

@uilianries: this is a weird case, we don't want to have an interval defined without a file that tells what needs to be conan config install

@@ -521,3 +522,53 @@ def get_zip():
self.client.run("config install http://localhost:%s/myconfig.zip" % http_server.port)
self.assertIn("Unzipping", self.client.out)
http_server.stop()

def test_config_install_sched_error(self):
Copy link
Member

@memsharded memsharded Apr 28, 2020

Choose a reason for hiding this comment

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

Better separate in independent tests for unrelated checks.

@memsharded memsharded requested a review from jgsogo Apr 28, 2020
@memsharded
Copy link
Member

@memsharded memsharded commented Apr 28, 2020

I would expect it to run with the search command at 12:40am (> 11:30am +1h), but it didn't because config_install.json has been modified at 12:15am.

I disagree. I don't think this should be a cron job. The expected next time to run is 13:15, exactly because you run a config install at 12:15. If you run a config install always 1 min before the scheduled automatic call, you never get automatic calls.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Apr 28, 2020

I would expect it to run with the search command at 12:40am (> 11:30am +1h), but it didn't because config_install.json has been modified at 12:15am.

I disagree. I don't think this should be a cron job. The expected next time to run is 13:15, exactly because you run a config install at 12:15. If you run a config install always 1 min before the scheduled automatic call, you never get automatic calls.

The problem I see in my scenario is that you don't get the changes from myrepo1 when expected. Not a big problem, just to know what to expect/answer when issues arrive 😛

@memsharded
Copy link
Member

@memsharded memsharded commented Apr 28, 2020

The problem I see in my scenario is that you don't get the changes from myrepo1 when expected. Not a big problem, just to know what to expect/answer when issues arrive

I see, that is true, but I am fine with that scenario. Once you start doing manual calls to conan config install, you are overriding the scheduled auto-config-install.

I think there will be issues with either of the approaches, and we will be getting issues if the auto conan config install is fired just after manual calling conan config install.

Copy link
Member

@jgsogo jgsogo left a comment

I think it should work fine right now. Take into account the comments that are pending, but I think we can start to think about the docs.

conans/client/conf/__init__.py Outdated Show resolved Hide resolved
client.run('config install "%s"' % folder)
self.assertIn("Processing conan.conf", client.out)
content = load(client.cache.conan_conf_path)
self.assertEqual(1, content.count("config_install_interval"))
Copy link
Member

@jgsogo jgsogo Apr 28, 2020

Choose a reason for hiding this comment

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

This would be true if the file contains

# config_install_interval = 1h

which is the default.

Copy link
Member

@memsharded memsharded Apr 28, 2020

Choose a reason for hiding this comment

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

This is counting the name of occurrences of the literal "config_install_interval", not the value

Copy link
Member

@jgsogo jgsogo Apr 28, 2020

Choose a reason for hiding this comment

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

It should. The default content of conan.conf contains 1 appearance of the config_install_interval too. I suppose that we want to check that the command runs successfully and it contains the content of the configuration in folder, not the default one.

Copy link
Member

@memsharded memsharded Apr 28, 2020

Choose a reason for hiding this comment

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

Yes, please @uilianries add that check

Copy link
Member Author

@uilianries uilianries Apr 28, 2020

Choose a reason for hiding this comment

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

config_install_interval = 1h

No, when is commented the content.count returns 0.

Copy link
Member

@memsharded memsharded Apr 28, 2020

Choose a reason for hiding this comment

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

It shouldn't matter if commented or not, it is a literal string search content = load(client.cache.conan_conf_path)

The goal is to validate that the new conan.conf contains config_install_interval = 5m, because it was installed from the folder

memsharded and others added 2 commits Apr 28, 2020
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

@uilianries uilianries commented Apr 28, 2020

@memsharded I've split all tests.

uilianries added 2 commits Apr 28, 2020
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@memsharded memsharded requested a review from jgsogo Apr 28, 2020
@memsharded memsharded self-assigned this Apr 28, 2020
@memsharded memsharded merged commit 6704318 into conan-io:develop Apr 29, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants