Opened 8 years ago

Closed 8 years ago

#2113 closed defect (complete)

Configuration plugin check for datasources

Reported by: vorner Owned by: vorner
Priority: medium Milestone: Sprint-20120807
Component: configuration Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 4.27 Internal?: no

Description

Once the #1976 is merged, the config manager plugin for checking data sources will be present, but the check itself is a dummy allowing anything. The check should be written, probably using the #2051 (by trying to load the thing without cache and doing some more sanity checks, like the master files (that would be ignored there) actually exist).

Subtickets

Change History (13)

comment:1 Changed 8 years ago by shane

  • Estimated Difficulty changed from 0 to 5

comment:2 Changed 8 years ago by shane

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120717

comment:3 Changed 8 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:4 Changed 8 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Hello

It is ready for review. I don't think it needs a changelog, as it is mostly a part of the #1976 functionality and it was not released yet. There's a small bugfix from unrelated part of the work to make it work. The fix has no dedicated test, though ‒ I couldn't reproduce it with any reasonably small spec file, so I gave up. If you can come up with one, I will be happy.

Thank you

comment:5 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:6 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

The code looks ok, just a few comments:

I think the error string in case of bad classes could be better; it prints "Unrecognized RR parameter string" which isn't really helpful IMO.

I also found a small problem with the 'cache disabled' warning I made you add in a previous review; I added a MasterFiles? configuration with my zone and loaded it, but the ClientList? logs a warning:
2012-07-31 11:30:46.507 WARN [b10-cfgmgr.datasrc] DATASRC_LIST_NOT_CACHED zone tjeb.nl/IN not cached, cache disabled globally. Will not be available.

However, the zone appears to be loaded and served fine.

2012-07-31 11:35:39.506 WARN  [b10-cfgmgr.datasrc] DATASRC_LIST_NOT_CACHED zone tjeb.nl/IN not cached, cache disabled globally. Will not be available.

Looks like that is because it isn't called with allow_cache=True in datasrc_config_plugin.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

Hello

Replying to jelte:

I think the error string in case of bad classes could be better; it prints "Unrecognized RR parameter string" which isn't really helpful IMO.

Is it better now?

I also found a small problem with the 'cache disabled' warning I made you add in a previous review; I added a MasterFiles? configuration with my zone and loaded it, but the ClientList? logs a warning:
2012-07-31 11:30:46.507 WARN [b10-cfgmgr.datasrc] DATASRC_LIST_NOT_CACHED zone tjeb.nl/IN not cached, cache disabled globally. Will not be available.

However, the zone appears to be loaded and served fine.

2012-07-31 11:35:39.506 WARN  [b10-cfgmgr.datasrc] DATASRC_LIST_NOT_CACHED zone tjeb.nl/IN not cached, cache disabled globally. Will not be available.

Looks like that is because it isn't called with allow_cache=True in datasrc_config_plugin.

Yes, that's correct ‒ the config manager has the cache disabled, therefore it warns. But the auth has it enabled and it serves it.

But I don't think it is a good idea to enable the cache in the configuration manager, because then, it would take a long time to load it (imagine a very large zone) and then it would just throw it away anyway.

So, what do we do about the warning? I probably could mangle the configuration to not contain the zones and check them manually only. Would that be OK or does it sounds like too much of a hack?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

I think the error string in case of bad classes could be better; it prints "Unrecognized RR parameter string" which isn't really helpful IMO.

Is it better now?

yes :) thanks

Yes, that's correct ‒ the config manager has the cache disabled, therefore it warns. But the auth has it enabled and it serves it.

But I don't think it is a good idea to enable the cache in the configuration manager, because then, it would take a long time to load it (imagine a very large zone) and then it would just throw it away anyway.

So, what do we do about the warning? I probably could mangle the configuration to not contain the zones and check them manually only. Would that be OK or does it sounds like too much of a hack?

I was about to say that i'd prefer some form of 'dry run', where everything is checked but not actually loaded. But I guess that is the whole purpose of the config plugin in the first place.

Since masterfiles is already treated as a special case, could we just check whether the files exist and are readable (and not pass it to configure() if type is masterfiles)? Masterfiles is already special-cased in both the checker plugin and the client list.

Of course, that is assuming that this is the only one where caching is needed to serve something at all.

comment:9 in reply to: ↑ 8 Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

Hello

Replying to jelte:

Replying to vorner:

So, what do we do about the warning? I probably could mangle the configuration to not contain the zones and check them manually only. Would that be OK or does it sounds like too much of a hack?

I was about to say that i'd prefer some form of 'dry run', where everything is checked but not actually loaded. But I guess that is the whole purpose of the config plugin in the first place.

Yes, that's the dry run ;-). Well, it assumes most data sources are just databases and they are cheap to initialize.

Since masterfiles is already treated as a special case, could we just check whether the files exist and are readable (and not pass it to configure() if type is masterfiles)? Masterfiles is already special-cased in both the checker plugin and the client list.

Well, that's mostly what I meant by the hack. I do pass the data source there, for two reasons:

  • It is easier to replace the parameters than remove the element from the list
  • It still can check other parameters, if there ever are more general ones (like cache parameters, or something)

comment:10 Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

Ok, let's do that then

comment:11 Changed 8 years ago by jelte

  • Owner changed from vorner to jelte

ah you're faster than i realized, and have implemented something already :)

comment:12 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

Ok, this looks fine.

One possible thing is that in the test, you could also do a deepcopy and compare the possibly changed data to the copy, instead of converting it to JSON (it would slightly changed what is tested; for the elements it would test object equality versus json-representation-equality), but no strong opinion here.

Either way I think this can be merged.

comment:13 in reply to: ↑ 12 Changed 8 years ago by vorner

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 4.27

Hello

Replying to jelte:

One possible thing is that in the test, you could also do a deepcopy and compare the possibly changed data to the copy, instead of converting it to JSON (it would slightly changed what is tested; for the elements it would test object equality versus json-representation-equality), but no strong opinion here.

I'm not sure if the equal thing checks value equivalence or pointer equivalence. Maybe it's value, but I just didn't want to care and JSON is good too.

Anyway, merged.

Note: See TracTickets for help on using tickets.