Opened 8 years ago

Closed 8 years ago

#1378 closed task (complete)

Configuration of #213

Reported by: vorner Owned by: vorner
Priority: medium Milestone: Sprint-20111122
Component: ~Boss of BIND (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is another sub-branch of #213 for review. It holds the configuration changes (these are user visible). Some of the original tests were removed, because they manipulate with internal variables which are no longer present and are related to the old configuration way.

There's a workaround in the systest, but there should be a fix for it in master already, so I'm going to remove it on merge.

The dropping of privilegest doesn't work right now, it will be re-added in the next subbranch.

The git branch is trac213-incremental-config.

Subtickets

Change History (11)

comment:1 Changed 8 years ago by vorner

  • Status changed from new to reviewing

Ah, I forgot to put it into review.

comment:2 Changed 8 years ago by jinmei

can you specify the start point for this branch?

comment:3 Changed 8 years ago by vorner

Ah, sorry. The one before is 1d9614bc52634bd512121f34af66290a2cdb2958 (or you can say git diff trac213-incremental-restarts...).

Thanks

comment:4 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:5 follow-up: Changed 8 years ago by jinmei

Overall it looks fine. I have some relatively minor comments and questions.

bind10_src.py.in

  • read_bind10_config and start_all_processes are now both sufficiently small and could be combined into a single method. The only reason we don't do this seems to be the convenience for tests, and, if that's the reason I'd explicitly add a note to read_bind10_config (otherwise someone would try "refactoring" the code later). Also, if that's the reason we might want to make read_bind10_config "protected" by renaming it to _read_bind10_config.
  • start_all_processes: not really for this branch, but I'd make this method a bit more concise by removing mostly redundant temporary variable:
            #c_channel_env = self.c_channel_env => remove this line
            self.start_ccsession(self.c_channel_env)
    

unit tests

  • MockBob?.start_simple(): why this change?
    -                    'b10-xfrin': self.start_xfrin,
    
    (why was xfrin removed while keeping others?)
  • bind10_test: I don't think removing test_start(none/auth/resolver/both) is really correct (maybe except for none), because in test_config_start we only check the code path from config_handler while test_start variants check the one from start_all_processes (and testing these cases shouldn't be difficult anyway).
  • test_config_start_once: what's the purpose of this addition?
    +        bob._BoB_started = True
    
    (same for test_start_dhcp, etc)


  • TestBossComponents?: is there anything that has changed from the original trac213 tests? (I'm asking whether I need to review this again)

system tests
I guess this hack is due to a bug already fixed in master (forgot the
ticket number):

+echo 'config add Boss/components x
+config remove Boss/components x

I'd rather merge that fix here and remove this kludge now.

comment:6 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

bind10_src.py.in

  • read_bind10_config and start_all_processes are now both sufficiently small and could be combined into a single method. The only reason we don't do this seems to be the convenience for tests, and, if that's the reason I'd explicitly add a note to read_bind10_config (otherwise someone would try "refactoring" the code later). Also, if that's the reason we might want to make read_bind10_config "protected" by renaming it to _read_bind10_config.

Well, I didn't really think about it until you pointed it out, but yes, it's good for tests. I added the note.

unit tests

  • MockBob?.start_simple(): why this change?
    -                    'b10-xfrin': self.start_xfrin,
    
    (why was xfrin removed while keeping others?)

Because xfrin is currently special component (until we do something with the workaround with environment variables), so start_simple is not used to start it (and in fact shouldn't, so if it was used by accident, this would complain).

It should be readded once the workaround is not needed and it's no longer special.

  • bind10_test: I don't think removing test_start(none/auth/resolver/both) is really correct (maybe except for none), because in test_config_start we only check the code path from config_handler while test_start variants check the one from start_all_processes (and testing these cases shouldn't be difficult anyway).

OK, I added them back (or something that should be equivalent with them).

  • test_config_start_once: what's the purpose of this addition?
    +        bob._BoB_started = True
    
    (same for test_start_dhcp, etc)

Since the boss ignores configuration updates until it is fully started and it uses this variable to know it was started. We don't do the real startup, so we need to fake it.

  • TestBossComponents?: is there anything that has changed from the original trac213 tests? (I'm asking whether I need to review this again)

I copy-pasted it from the original version. I'm not 100% sure if I needed to update any variable names which might have changed in between, but they are the same otherwise.

system tests
I guess this hack is due to a bug already fixed in master (forgot the
ticket number):

+echo 'config add Boss/components x
+config remove Boss/components x

I'd rather merge that fix here and remove this kludge now.

Yes, I know about that one. I didn't want to bring in another merge which would make the review harder. I want to remove it at the merge with master (or, I'd propose waiting at last until the branch stabilizes, just because of the reviews). I don't want this to exist in master.

Thanks.

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

Replying to vorner:

OK, I added them back (or something that should be equivalent with them).

  • test_config_start_once: what's the purpose of this addition?
    +        bob._BoB_started = True
    
    (same for test_start_dhcp, etc)

Since the boss ignores configuration updates until it is fully started and it uses this variable to know it was started. We don't do the real startup, so we need to fake it.

But the test passes even without this setting (btw, the same thing
applies to runnable). And, I now realized that even if we really
needed it it shouldn't have been correct. It should be
_BoB__started, not _BoB_started. There should be some confustion
here.

system tests
I guess this hack is due to a bug already fixed in master (forgot the
ticket number):

+echo 'config add Boss/components x
+config remove Boss/components x

I'd rather merge that fix here and remove this kludge now.

Yes, I know about that one. I didn't want to bring in another merge which would make the review harder. I want to remove it at the merge with master (or, I'd propose waiting at last until the branch stabilizes, just because of the reviews). I don't want this to exist in master.

Okay, then please make sure it's cleaned up on merge.

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:10 in reply to: ↑ 7 Changed 8 years ago by jinmei

Replying to vorner:

  • TestBossComponents?: is there anything that has changed from the original trac213 tests? (I'm asking whether I need to review this again)

I copy-pasted it from the original version. I'm not 100% sure if I needed to update any variable names which might have changed in between, but they are the same otherwise.

I noiced one thing (not actually specific to this test): in another branch
we renambed BoB.processes to BoB.components. So we should eventually be
consistent with the naming in the tests (for MockBob?, and as a result
in TestBossComponents?).

In my first review I only made one comment for this test, which seemed
to be addressed. I've not looked into it any closer, but from a quick
glance I've not found anything obvious wrong except for s/processes/components/.

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

  • Resolution set to complete
  • Status changed from reviewing to closed

Hello

Replying to jinmei:

Since the boss ignores configuration updates until it is fully started and it uses this variable to know it was started. We don't do the real startup, so we need to fake it.

But the test passes even without this setting (btw, the same thing
applies to runnable). And, I now realized that even if we really
needed it it shouldn't have been correct. It should be
_BoB__started, not _BoB_started. There should be some confustion
here.

After looking at it, there was one test that needed it (and that one did have two underscores). I probably just copy-pasted it into all the similar tests just to make sure then, but it seems it is not needed, so I removed it.

Anyway, everything here was addressed and merged into the #213 branch, so closing this subtask.

Note: See TracTickets for help on using tickets.